ir: Fix incorrect renaming of phinode values (#52614)

This fixes #52610. The underlying issue is a left over OldSSAValue after
the adce_pass! (introduced by compaction, it being during adce is
incidental). Compaction introduces `OldSSAValue` when it compacts in
PhiNodes that reference later SSAValues and adds them to a list to
revisit at the end of compaction to fill in the actual renamed result.
There are two separate fixes here:

1. If the result of the final revisit is yet another `OldSSAValue`,
rename it again. I don't this ordinarily happens at all, but I suppose
it is possible in theory during sroa beacuse of the rename-shortcut
optimization [1]. However, this is not not what happened here. Instead
compaction incorrectly used an OldSSAValue for an already-inserted node,
which then ends up in the rename list because we deleted one of the
predecessor edges [2]. To fix that we:

2. Fix an issue where we weren't accounting for the possibility of
previously pending nodes (which have SSAValues beyond the numbering
range of the ordinary statements) in the special already_inserted query
in phinode value processing. To fix this, unify the logic with the
ordinary `already_inserted` query, which handles this case correctly.

[1]
https://github.com/JuliaLang/julia/blob/9443c761871c4db9c3213a1e01804286292c3f4d/base/compiler/ssair/passes.jl#L1385
[2]
https://github.com/JuliaLang/julia/blob/9443c761871c4db9c3213a1e01804286292c3f4d/base/compiler/ssair/ir.jl#L1556

Co-authored-by: Tim Besard <[email protected]>

Author: Keno

base/compiler/ssair/ir.jl

@@ -1359,7 +1359,7 @@ function process_node!(compact::IncrementalCompact, result_idx::Int, inst::Instr
     (; result, ssa_rename, late_fixup, used_ssas, new_new_used_ssas) = compact
     (; cfg_transforms_enabled, fold_constant_branches, bb_rename_succ, bb_rename_pred, result_bbs) = compact.cfg_transform
     mark_refined! = Refiner(result.flag, result_idx)
-    already_inserted = (::Int, ssa::OldSSAValue)->ssa.id <= processed_idx
+    already_inserted_phi_arg = already_inserted_ssa(compact, processed_idx)
     if stmt === nothing
         ssa_rename[idx] = stmt
     elseif isa(stmt, OldSSAValue)
@@ -1535,7 +1535,7 @@ function process_node!(compact::IncrementalCompact, result_idx::Int, inst::Instr
             values = stmt.values
         end
 
-        values = process_phinode_values(values, late_fixup, already_inserted, result_idx, ssa_rename, used_ssas, new_new_used_ssas, do_rename_ssa, mark_refined!)
+        values = process_phinode_values(values, late_fixup, already_inserted_phi_arg, result_idx, ssa_rename, used_ssas, new_new_used_ssas, do_rename_ssa, mark_refined!)
         # 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
@@ -1574,7 +1574,7 @@ function process_node!(compact::IncrementalCompact, result_idx::Int, inst::Instr
                 push!(values, value)
             end
         end
-        result[result_idx][:stmt] = PhiCNode(process_phinode_values(values, late_fixup, already_inserted, result_idx, ssa_rename, used_ssas, new_new_used_ssas, do_rename_ssa, mark_refined!))
+        result[result_idx][:stmt] = PhiCNode(process_phinode_values(values, late_fixup, already_inserted_phi_arg, result_idx, ssa_rename, used_ssas, new_new_used_ssas, do_rename_ssa, mark_refined!))
         result_idx += 1
     else
         if isa(stmt, SSAValue)
@@ -1883,6 +1883,8 @@ function fixup_node(compact::IncrementalCompact, @nospecialize(stmt), reify_new_
             compact.used_ssas[node.id] += 1
         elseif isa(node, NewSSAValue)
             compact.new_new_used_ssas[-node.id] += 1
+        elseif isa(node, OldSSAValue)
+            return fixup_node(compact, node, reify_new_nodes)
         end
         return FixedNode(node, needs_fixup)
     else

base/compiler/ssair/passes.jl

@@ -348,17 +348,23 @@ function record_immutable_preserve!(new_preserves::Vector{Any}, def::Expr, compa
 end
 
 function already_inserted(compact::IncrementalCompact, old::OldSSAValue)
-    id = old.id
-    if id < length(compact.ir.stmts)
-        return id < compact.idx
-    end
-    id -= length(compact.ir.stmts)
-    if id < length(compact.ir.new_nodes)
-        return already_inserted(compact, OldSSAValue(compact.ir.new_nodes.info[id].pos))
+    already_inserted_ssa(compact, compact.idx-1)(0, old)
+end
+
+function already_inserted_ssa(compact::IncrementalCompact, processed_idx::Int)
+    return function did_already_insert(phi_arg::Int, old::OldSSAValue)
+        id = old.id
+        if id <= length(compact.ir.stmts)
+            return id <= processed_idx
+        end
+        id -= length(compact.ir.stmts)
+        if id <= length(compact.ir.new_nodes)
+            return did_already_insert(phi_arg, OldSSAValue(compact.ir.new_nodes.info[id].pos))
+        end
+        id -= length(compact.ir.new_nodes)
+        @assert id <= length(compact.pending_nodes)
+        return !(id in compact.pending_perm)
     end
-    id -= length(compact.ir.new_nodes)
-    @assert id <= length(compact.pending_nodes)
-    return !(id in compact.pending_perm)
 end
 
 function is_pending(compact::IncrementalCompact, old::OldSSAValue)
@@ -2079,6 +2085,7 @@ function adce_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
             end
         end
     end
+
     return Pair{IRCode, Bool}(complete(compact), made_changes)
 end
 

base/compiler/ssair/verify.jl

@@ -72,7 +72,7 @@ function check_op(ir::IRCode, domtree::DomTree, @nospecialize(op), use_bb::Int,
             end
         end
     elseif isa(op, Union{OldSSAValue, NewSSAValue})
-        @verify_error "Left over SSA marker"
+        @verify_error "At statement %$use_idx: Left over SSA marker ($op)"
         error("")
     elseif isa(op, SlotNumber)
         @verify_error "Left over slot detected in converted IR"

test/compiler/irpasses.jl

@@ -1709,3 +1709,19 @@ end
 @test scope_folding_opt() == 1
 @test_broken fully_eliminated(scope_folding)
 @test_broken fully_eliminated(scope_folding_opt)
+
+# Function that happened to have lots of sroa that
+# happened to trigger a bad case in the renamer. We
+# just want to check this doesn't crash in inference.
+function f52610()
+    slots_dict = IdDict()
+    for () in Base.inferencebarrier(1)
+       for x in 1
+           if Base.inferencebarrier(true)
+               slots_dict[x] = 0
+           end
+       end
+    end
+    return nothing
+end
+@test code_typed(f52610)[1][2] === Nothing