Skip to content

Commit

Permalink
cfg_simplify: Correctly handle must-throw blocks (JuliaLang#54216)
Browse files Browse the repository at this point in the history
We have a special exception in our IR that allows `::Union{}` marked
instructions to act as an `unreachable` terminator.

Without this fix, we were advancing into the subsequent block despite it
not being a successor.
  • Loading branch information
topolarity authored Apr 25, 2024
1 parent 96866cb commit 729a8c9
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 2 deletions.
16 changes: 14 additions & 2 deletions base/compiler/ssair/passes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2274,6 +2274,15 @@ function cfg_simplify!(ir::IRCode)
elseif merge_into[idx] == 0 && is_bb_empty(ir, bb) && is_legal_bb_drop(ir, idx, bb)
# If this BB is empty, we can still merge it as long as none of our successor's phi nodes
# reference our predecessors.
#
# This is for situations like:
# #1 - ...
# goto #3 if not ...
# #2 - (empty)
# #3 - ϕ(#2 => true, #1 => false)
#
# where we rely on the empty basic block to disambiguate the ϕ-node's value

found_interference = false
preds = Int[ascend_eliminated_preds(bbs, pred) for pred in bb.preds]
for idx in bbs[succ].stmts
Expand Down Expand Up @@ -2331,8 +2340,11 @@ function cfg_simplify!(ir::IRCode)
end
curr = merged_succ[curr]
end
terminator = ir[SSAValue(ir.cfg.blocks[curr].stmts[end])][:stmt]
if isa(terminator, GotoNode) || isa(terminator, ReturnNode)
terminator = ir[SSAValue(bbs[curr].stmts[end])][:stmt]
is_throw = ir[SSAValue(bbs[curr].stmts[end])][:type] === Union{} && !isa(terminator, PhiNode)
if isa(terminator, GotoNode) || isa(terminator, ReturnNode) || is_throw
# Only advance to next block if it's a successor
# (avoid GotoNode, ReturnNode, throw()/Union{})
break
elseif isa(terminator, GotoIfNot)
if bb_rename_succ[terminator.dest] == 0
Expand Down
30 changes: 30 additions & 0 deletions test/compiler/irpasses.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1881,3 +1881,33 @@ end
@test !fully_eliminated() do
all(iszero, Iterators.repeated(0))
end

## Test that cfg_simplify respects implicit `unreachable` terminators
let code = Any[
# block 1
GotoIfNot(Core.Argument(2), 4),
# block 2
Expr(:call, Base.throw, "error"), # an implicit `unreachable` terminator
# block 3
Expr(:call, :opaque),
# block 4
ReturnNode(nothing),
]
ir = make_ircode(code; ssavaluetypes=Any[Union{}, Union{}, Any, Union{}])

# Unfortunately `compute_basic_blocks` does not notice the `throw()` so it gives us
# a slightly imprecise CFG. Instead manually construct the CFG we need for this test:
empty!(ir.cfg.blocks)
push!(ir.cfg.blocks, BasicBlock(StmtRange(1,1), [], [2,4]))
push!(ir.cfg.blocks, BasicBlock(StmtRange(2,2), [1], []))
push!(ir.cfg.blocks, BasicBlock(StmtRange(3,3), [], []))
push!(ir.cfg.blocks, BasicBlock(StmtRange(4,4), [1], []))
empty!(ir.cfg.index)
append!(ir.cfg.index, Int[2,3,4])
ir.stmts.stmt[1] = GotoIfNot(Core.Argument(2), 4)

Core.Compiler.verify_ir(ir)
ir = Core.Compiler.cfg_simplify!(ir)
Core.Compiler.verify_ir(ir)
@test length(ir.cfg.blocks) == 3 # should have removed block 3
end

0 comments on commit 729a8c9

Please sign in to comment.