ir: Fix incorrect renaming of phinode values

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/master/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]>

base/compiler/ssair/ir.jl

@@ -1363,7 +1363,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)
@@ -1539,7 +1539,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
@@ -1578,7 +1578,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)
@@ -1887,6 +1887,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)
@@ -2092,6 +2098,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

@@ -1697,3 +1697,17 @@ end
 @test scope_folding_opt() == 1
 @test_broken fully_eliminated(scope_folding)
 @test_broken fully_eliminated(scope_folding_opt)
+
+# Function that happened to happens to have lots of sroa that
+# happened to trigger a bad case in the renamer.
+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