Skip to content

Commit

Permalink
sroa: Fix affinity of inserted undef check
Browse files Browse the repository at this point in the history
There is a bit of complexity here making sure that this instruction
ends up in the correct basic block because there's a fair number
of corner cases (e.g. whether we're deleting the first instruction
of a basic block or the last or both). Side-step all that by
never doing the delete-then-insert dance and instead doing regular
instruction replacement if necessary. Fixes #52857.
  • Loading branch information
Keno committed Jan 12, 2024
1 parent 5b6a94d commit bc7cfde
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 7 deletions.
20 changes: 13 additions & 7 deletions base/compiler/ssair/passes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1403,7 +1403,7 @@ function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
(lifted_val, nest) = perform_lifting!(compact,
visited_philikes, field, result_t, lifted_leaves, val, lazydomtree)

node_was_deleted = false
should_delete_node = false
line = compact[SSAValue(idx)][:line]
if lifted_val !== nothing && !(𝕃ₒ, compact[SSAValue(idx)][:type], result_t)
compact[idx] = lifted_val === nothing ? nothing : lifted_val.val
Expand All @@ -1412,9 +1412,8 @@ function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
# Save some work in a later compaction, by inserting this into the renamer now,
# but only do this if we didn't set the REFINED flag, to save work for irinterp
# in revisiting only the renamings that came through *this* idx.
delete_inst_here!(compact)
compact.ssa_rename[old_idx] = lifted_val === nothing ? nothing : lifted_val.val
node_was_deleted = true
should_delete_node = true
else
compact[idx] = lifted_val === nothing ? nothing : lifted_val.val
end
Expand All @@ -1435,17 +1434,24 @@ function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
def_val = (def_val::LiftedValue).val
finish_phi_nest!(compact, nest)
end
ni = NewInstruction(
Expr(:throw_undef_if_not, Symbol("##getfield##"), def_val), Nothing, line)
if node_was_deleted
insert_node_here!(compact, ni, true)
throw_expr = Expr(:throw_undef_if_not, Symbol("##getfield##"), def_val)
if should_delete_node
# Replace the node we already have rather than deleting/re-inserting.
# This way it is easier to handle BB boundary corner cases.
compact[SSAValue(idx)] = throw_expr
compact[SSAValue(idx)][:type] = Nothing
compact[SSAValue(idx)][:flag] = IR_FLAG_EFFECT_FREE | IR_FLAG_CONSISTENT | IR_FLAG_NOUB
should_delete_node = false
else
ni = NewInstruction(throw_expr, Nothing, line)
insert_node!(compact, SSAValue(idx), ni)
end
else
# val must be defined
@assert lifted_val !== nothing
end

should_delete_node && delete_inst_here!(compact)
end

non_dce_finish!(compact)
Expand Down
14 changes: 14 additions & 0 deletions test/compiler/irpasses.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1764,3 +1764,17 @@ let code = Any[
@test Core.Compiler.verify_ir(ir) === nothing
@test count(x->isa(x, GotoIfNot), ir.stmts.stmt) == 1
end

# Issue #52857 - Affinity of sroa definedness check
let code = Any[
Expr(:new, ImmutableRef{Any}),
GotoIfNot(Argument(1), 4),
Expr(:call, GlobalRef(Base, :getfield), SSAValue(1), 1), # Will throw
ReturnNode(1)
]
ir = make_ircode(code; ssavaluetypes = Any[ImmutableRef{Any}, Any, Any, Any], slottypes=Any[Bool], verify=true)
ir = Core.Compiler.sroa_pass!(ir)
@test Core.Compiler.verify_ir(ir) === nothing
@test !any(iscall((ir, getfield)), ir.stmts.stmt)
@test length(ir.cfg.blocks[end].stmts) == 1
end

0 comments on commit bc7cfde

Please sign in to comment.