From b600054d7e73f691a80e63b639c975c5df128c3c Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 22 Dec 2023 16:07:40 +0000 Subject: [PATCH] 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 --- base/compiler/ssair/ir.jl | 8 +++++--- base/compiler/ssair/passes.jl | 27 +++++++++++++++++---------- base/compiler/ssair/verify.jl | 2 +- test/compiler/irpasses.jl | 14 ++++++++++++++ 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/base/compiler/ssair/ir.jl b/base/compiler/ssair/ir.jl index 3a613f4170a10..9af6647cdbc58 100644 --- a/base/compiler/ssair/ir.jl +++ b/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 diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl index c16623c815b09..c9cdaa95b4dda 100644 --- a/base/compiler/ssair/passes.jl +++ b/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 diff --git a/base/compiler/ssair/verify.jl b/base/compiler/ssair/verify.jl index dfa6a4aad2281..13656683e8ea9 100644 --- a/base/compiler/ssair/verify.jl +++ b/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" diff --git a/test/compiler/irpasses.jl b/test/compiler/irpasses.jl index 97e80fca6cba1..ccba7efb03092 100644 --- a/test/compiler/irpasses.jl +++ b/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