Decline to remove phi nodes when this might cause forward references

Phi nodes are optimized away when there is only one predecessor, but this can
cause problems in dead loops because forward references can be created, leading
to issues with optimization passes that look at all code, dead or not. This
addresses issue JuliaLang#29107 when DCE is turned on.

base/compiler/ssair/ir.jl

@@ -459,16 +459,20 @@ end
 
 mutable struct IncrementalCompact
     ir::IRCode
+
     result::Vector{Any}
     result_types::Vector{Any}
     result_lines::Vector{Int32}
     result_flags::Vector{UInt8}
     result_bbs::Vector{BasicBlock}
+
     ssa_rename::Vector{Any}
     bb_rename_pred::Vector{Int}
     bb_rename_succ::Vector{Int}
+
     used_ssas::Vector{Int}
     late_fixup::Vector{Int}
+
     # This could be Stateful, but bootstrapping doesn't like that
     perm::Vector{Int}
     new_nodes_idx::Int
@@ -477,6 +481,7 @@ mutable struct IncrementalCompact
     # TODO: Switch these two to a min-heap of some sort
     pending_nodes::Vector{NewNode}  # New nodes that were after the compaction point at insertion time
     pending_perm::Vector{Int}
+
     # State
     idx::Int
     result_idx::Int
@@ -841,7 +846,8 @@ function kill_edge!(compact::IncrementalCompact, active_bb::Int, from::Int, to::
     # Note: We recursively kill as many edges as are obviously dead. However, this
     # may leave dead loops in the IR. We kill these later in a CFG cleanup pass (or
     # worstcase during codegen).
-    preds, succs = compact.result_bbs[compact.bb_rename_succ[to]].preds, compact.result_bbs[compact.bb_rename_pred[from]].succs
+    preds = compact.result_bbs[compact.bb_rename_succ[to]].preds
+    succs = compact.result_bbs[compact.bb_rename_pred[from]].succs
     deleteat!(preds, findfirst(x->x === compact.bb_rename_pred[from], preds)::Int)
     deleteat!(succs, findfirst(x->x === compact.bb_rename_succ[to], succs)::Int)
     # Check if the block is now dead
@@ -953,7 +959,15 @@ function process_node!(compact::IncrementalCompact, result::Vector{Any},
         result_idx += 1
     elseif isa(stmt, PhiNode)
         values = process_phinode_values(stmt.values, late_fixup, processed_idx, result_idx, ssa_rename, used_ssas, do_rename_ssa)
-        if length(stmt.edges) == 1 && isassigned(values, 1) &&
+        # Don't remove the phi node if it is before the definition of its value
+        # because doing so can create forward references. This should only
+        # happen with dead loops, but can cause problems when optimization
+        # passes look at all code, dead or not. This check should be
+        # unnecessary when DCE can remove those dead loops entirely, so this is
+        # just to be safe.
+        before_def = isassigned(values, 1) && isa(values[1], OldSSAValue) &&
+            idx < values[1].id
+        if length(stmt.edges) == 1 && isassigned(values, 1) && !before_def &&
                 length(compact.cfg_transforms_enabled ?
                     compact.result_bbs[compact.bb_rename_succ[active_bb]].preds :
                     compact.ir.cfg.blocks[active_bb].preds) == 1

test/compiler/ssair.jl

@@ -1,5 +1,6 @@
 # This file is a part of Julia. License is MIT: https://julialang.org/license
 
+using Base.Meta
 using Core.IR
 const Compiler = Core.Compiler
 
@@ -134,7 +135,6 @@ end
 @test f32579(0, false) === false
 
 # Test for bug caused by renaming blocks improperly, related to PR #32145
-using Base.Meta
 let ci = (Meta.@lower 1 + 1).args[1]
     ci.code = [
         # block 1
@@ -163,3 +163,40 @@ let ci = (Meta.@lower 1 + 1).args[1]
     ir = Core.Compiler.compact!(ir, true)
     @test Core.Compiler.verify_ir(ir) == nothing
 end
+
+# Issue #29107
+let ci = (Meta.@lower 1 + 1).args[1]
+    ci.code = [
+        # Block 1
+        Core.Compiler.GotoNode(6),
+        # Block 2
+        # The following phi node gets deleted because it only has one edge, so
+        # the call to `something` is made to use the value of `something2()`,
+        # even though this value is defined after it. We don't want this to
+        # happen even though this block is dead because subsequent optimization
+        # passes may look at all code, dead or not.
+        Core.PhiNode(Any[2], Any[Core.SSAValue(4)]),
+        Expr(:call, :something, Core.SSAValue(2)),
+        Expr(:call, :something2),
+        Core.Compiler.GotoNode(2),
+        # Block 3
+        Core.Compiler.ReturnNode(1000)
+    ]
+    nstmts = length(ci.code)
+    ci.ssavaluetypes = nstmts
+    ci.codelocs = fill(Int32(1), nstmts)
+    ci.ssaflags = fill(Int32(0), nstmts)
+    ir = Core.Compiler.inflate_ir(ci)
+    ir = Core.Compiler.compact!(ir, true)
+    # Make sure that if there is a call to `something` (block 2 should be
+    # removed entirely with working DCE), it doesn't use any SSA values that
+    # come after it.
+    for i in 1:length(ir.stmts)
+        s = ir.stmts[i]
+        if isa(s, Expr) && s.head == :call && s.args[1] == :something
+            if isa(s.args[2], SSAValue)
+                @test s.args[2].id <= i
+            end
+        end
+    end
+end