Skip to content

Commit

Permalink
cfg_simplify: Fix more bugs (#47240)
Browse files Browse the repository at this point in the history 
cfg_simplify still had issues with unreachable BBs, as well as BBs with
try/catch in them. Additionally, it sometimes leaves unreachable BBs
in the IR, which the verifier was upset about if there was a PhiNode
that referenced it. I made that legal for now. The alternative is to
make all unreachable BBs illegal, which I think would be reasonable,
but is somewhat extreme for the time being. Let's see how this
fares first.
  • Loading branch information
@Keno
Keno authored Oct 20, 2022
1 parent 7680f77 commit ecf9e56
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 34 deletions.
2 changes: 1 addition & 1 deletion base/compiler/ssair/ir.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ function compute_basic_blocks(stmts::Vector{Any})
end

# this function assumes insert position exists
function first_insert_for_bb(code, cfg::CFG, block::Int)
function first_insert_for_bb(code::Vector{Any}, cfg::CFG, block::Int)
for idx in cfg.blocks[block].stmts
stmt = code[idx]
if !isa(stmt, PhiNode)
Expand Down
36 changes: 33 additions & 3 deletions base/compiler/ssair/passes.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1840,6 +1840,8 @@ end

# TODO: This is terrible, we should change the IR for GotoIfNot to gain an else case
function is_legal_bb_drop(ir::IRCode, bbidx::Int, bb::BasicBlock)
# For the time being, don't drop the first bb, because it has special predecessor semantics.
bbidx == 1 && return false
# If the block we're going to is the same as the fallthrow, it's always legal to drop
# the block.
length(bb.stmts) == 0 && return true
Expand All @@ -1863,6 +1865,8 @@ function is_legal_bb_drop(ir::IRCode, bbidx::Int, bb::BasicBlock)
return true
end

is_terminator(@nospecialize(inst)) = isa(inst, GotoNode) || isa(inst, GotoIfNot) || isexpr(inst, :enter)

function cfg_simplify!(ir::IRCode)
bbs = ir.cfg.blocks
merge_into = zeros(Int, length(bbs))
Expand Down Expand Up @@ -1891,14 +1895,19 @@ function cfg_simplify!(ir::IRCode)
for (idx, bb) in enumerate(bbs)
if length(bb.succs) == 1
succ = bb.succs[1]
if length(bbs[succ].preds) == 1
if length(bbs[succ].preds) == 1 && succ != 1
# Can't merge blocks with :enter terminator even if they
# only have one successor.
if isexpr(ir[SSAValue(last(bb.stmts))][:inst], :enter)
continue
end
# Prevent cycles by making sure we don't end up back at `idx`
# by following what is to be merged into `succ`
if follow_merged_succ(succ) != idx
merge_into[succ] = idx
merged_succ[idx] = succ
end
elseif is_bb_empty(ir, bb) && is_legal_bb_drop(ir, idx, bb)
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.
found_interference = false
Expand All @@ -1919,6 +1928,21 @@ function cfg_simplify!(ir::IRCode)
end
@label done
if !found_interference
# Hack, but effective. If we have a predecessor with a fall-through terminator, change the
# instruction numbering to merge the blocks now such that below processing will properly
# update it.
if idx-1 in bb.preds
last_fallthrough = idx-1
dbi = length(dropped_bbs)
while dbi != 0 && dropped_bbs[dbi] == last_fallthrough && (last_fallthrough-1 in bbs[last_fallthrough].preds)
last_fallthrough -= 1
dbi -= 1
end
terminator = ir[SSAValue(last(bbs[last_fallthrough].stmts))][:inst]
if !is_terminator(terminator)
bbs[last_fallthrough] = BasicBlock(first(bbs[last_fallthrough].stmts):last(bb.stmts), bbs[last_fallthrough].preds, bbs[last_fallthrough].succs)
end
end
push!(dropped_bbs, idx)
end
end
Expand Down Expand Up @@ -1965,6 +1989,8 @@ function cfg_simplify!(ir::IRCode)
if bb_rename_succ[terminator.dest] == 0
push!(worklist, terminator.dest)
end
elseif isexpr(terminator, :enter)
push!(worklist, terminator.args[1])
end
ncurr = curr + 1
if !isempty(searchsorted(dropped_bbs, ncurr))
Expand Down Expand Up @@ -2087,8 +2113,12 @@ function cfg_simplify!(ir::IRCode)
res = Int[]
function scan_preds!(preds)
for pred in preds
if pred == 0
push!(res, 0)
continue
end
r = bb_rename_pred[pred]
r == -2 && continue
(r == -2 || r == -1) && continue
if r == -3
scan_preds!(bbs[pred].preds)
else
Expand Down
10 changes: 5 additions & 5 deletions base/compiler/ssair/slot2ssa.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -738,11 +738,11 @@ function construct_ssa!(ci::CodeInfo, ir::IRCode, domtree::DomTree,
for (slot, _, node) in phicnodes[catch_entry_blocks[eidx][2]]
ival = incoming_vals[slot_id(slot)]
ivalundef = ival === UNDEF_TOKEN
unode = ivalundef ? UpsilonNode() : UpsilonNode(ival)
typ = ivalundef ? MaybeUndef(Union{}) : typ_for_val(ival, ci, ir.sptypes, -1, slottypes)
push!(node.values,
NewSSAValue(insert_node!(ir, first_insert_for_bb(code, cfg, item),
NewInstruction(unode, typ), true).id - length(ir.stmts)))
Υ = NewInstruction(ivalundef ? UpsilonNode() : UpsilonNode(ival),
ivalundef ? MaybeUndef(Union{}) : typ_for_val(ival, ci, ir.sptypes, -1, slottypes))
# insert `UpsilonNode` immediately before the `:enter` expression
Υssa = insert_node!(ir, first_insert_for_bb(code, cfg, item), Υ)
push!(node.values, NewSSAValue(Υssa.id - length(ir.stmts)))
end
end
push!(visited, item)
Expand Down
77 changes: 52 additions & 25 deletions base/compiler/ssair/verify.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ if !isdefined(@__MODULE__, Symbol("@verify_error"))
end
end

function check_op(ir::IRCode, domtree::DomTree, @nospecialize(op), use_bb::Int, use_idx::Int, print::Bool, isforeigncall::Bool, arg_idx::Int, allow_frontend_forms::Bool)
function check_op(ir::IRCode, domtree::DomTree, @nospecialize(op), use_bb::Int, use_idx::Int, printed_use_idx::Int, print::Bool, isforeigncall::Bool, arg_idx::Int, allow_frontend_forms::Bool)
if isa(op, SSAValue)
if op.id > length(ir.stmts)
def_bb = block_for_inst(ir.cfg, ir.new_nodes.info[op.id - length(ir.stmts)].pos)
Expand All @@ -39,7 +39,7 @@ function check_op(ir::IRCode, domtree::DomTree, @nospecialize(op), use_bb::Int,
else
if !dominates(domtree, def_bb, use_bb) && !(bb_unreachable(domtree, def_bb) && bb_unreachable(domtree, use_bb))
# At the moment, we allow GC preserve tokens outside the standard domination notion
@verify_error "Basic Block $def_bb does not dominate block $use_bb (tried to use value $(op.id))"
@verify_error "Basic Block $def_bb does not dominate block $use_bb (tried to use value %$(op.id) at %$(printed_use_idx))"
error("")
end
end
Expand Down Expand Up @@ -85,20 +85,14 @@ function verify_ir(ir::IRCode, print::Bool=true, allow_frontend_forms::Bool=fals
# @assert isempty(ir.new_nodes)
# Verify CFG
last_end = 0
# Verify statements
domtree = construct_domtree(ir.cfg.blocks)
# Verify CFG graph. Must be well formed to construct domtree
for (idx, block) in pairs(ir.cfg.blocks)
if first(block.stmts) != last_end + 1
#ranges = [(idx,first(bb.stmts),last(bb.stmts)) for (idx, bb) in pairs(ir.cfg.blocks)]
@verify_error "First statement of BB $idx ($(first(block.stmts))) does not match end of previous ($last_end)"
error("")
end
last_end = last(block.stmts)
terminator = ir.stmts[last_end][:inst]

bb_unreachable(domtree, idx) && continue
for p in block.preds
p == 0 && continue
if !(1 <= p <= length(ir.cfg.blocks))
@verify_error "Predecessor $p of block $idx out of bounds for IR"
error("")
end
c = count_int(idx, ir.cfg.blocks[p].succs)
if c == 0
@verify_error "Predecessor $p of block $idx not in successor list"
Expand All @@ -110,6 +104,32 @@ function verify_ir(ir::IRCode, print::Bool=true, allow_frontend_forms::Bool=fals
end
end
end
for s in block.succs
if !(1 <= s <= length(ir.cfg.blocks))
@verify_error "Successor $s of block $idx out of bounds for IR"
error("")
end
if !(idx in ir.cfg.blocks[s].preds)
#@Base.show ir.cfg
#@Base.show ir
#@Base.show ir.argtypes
@verify_error "Successor $s of block $idx not in predecessor list"
error("")
end
end
end
# Verify statements
domtree = construct_domtree(ir.cfg.blocks)
for (idx, block) in pairs(ir.cfg.blocks)
if first(block.stmts) != last_end + 1
#ranges = [(idx,first(bb.stmts),last(bb.stmts)) for (idx, bb) in pairs(ir.cfg.blocks)]
@verify_error "First statement of BB $idx ($(first(block.stmts))) does not match end of previous ($last_end)"
error("")
end
last_end = last(block.stmts)
terminator = ir.stmts[last_end][:inst]

bb_unreachable(domtree, idx) && continue
if isa(terminator, ReturnNode)
if !isempty(block.succs)
@verify_error "Block $idx ends in return or unreachable, but has successors"
Expand Down Expand Up @@ -151,15 +171,6 @@ function verify_ir(ir::IRCode, print::Bool=true, allow_frontend_forms::Bool=fals
error("")
end
end
for s in block.succs
if !(idx in ir.cfg.blocks[s].preds)
#@Base.show ir.cfg
#@Base.show ir
#@Base.show ir.argtypes
@verify_error "Successor $s of block $idx not in predecessor list"
error("")
end
end
end
for (bb, idx) in bbidxiter(ir)
# We allow invalid IR in dead code to avoid passes having to detect when
Expand All @@ -186,6 +197,12 @@ function verify_ir(ir::IRCode, print::Bool=true, allow_frontend_forms::Bool=fals
error("")
end
edge == 0 && continue
if bb_unreachable(domtree, Int64(edge))
# TODO: Disallow?
#@verify_error "Unreachable edge from #$edge should have been cleaned up at idx $idx"
#error("")
continue
end
isassigned(stmt.values, i) || continue
val = stmt.values[i]
phiT = ir.stmts[idx][:type]
Expand All @@ -199,7 +216,7 @@ function verify_ir(ir::IRCode, print::Bool=true, allow_frontend_forms::Bool=fals
#error("")
end
end
check_op(ir, domtree, val, Int(edge), last(ir.cfg.blocks[stmt.edges[i]].stmts)+1, print, false, i, allow_frontend_forms)
check_op(ir, domtree, val, Int(edge), last(ir.cfg.blocks[stmt.edges[i]].stmts)+1, idx, print, false, i, allow_frontend_forms)
end
elseif isa(stmt, PhiCNode)
for i = 1:length(stmt.values)
Expand All @@ -213,11 +230,21 @@ function verify_ir(ir::IRCode, print::Bool=true, allow_frontend_forms::Bool=fals
error("")
end
end
elseif (isa(stmt, GotoNode) || isa(stmt, GotoIfNot) || isexpr(stmt, :enter)) && idx != last(ir.cfg.blocks[bb].stmts)
@verify_error "Terminator $idx in bb $bb is not the last statement in the block"
error("")
else
if isa(stmt, Expr) || isa(stmt, ReturnNode) # TODO: make sure everything has line info
if (stmt isa ReturnNode)
if isdefined(stmt, :val)
# TODO: Disallow unreachable returns?
# bb_unreachable(domtree, Int64(edge))
else
#@verify_error "Missing line number information for statement $idx of $ir"
end
end
if !(stmt isa ReturnNode && !isdefined(stmt, :val)) # not actually a return node, but an unreachable marker
if ir.stmts[idx][:line] <= 0
#@verify_error "Missing line number information for statement $idx of $ir"
end
end
end
Expand Down Expand Up @@ -254,7 +281,7 @@ function verify_ir(ir::IRCode, print::Bool=true, allow_frontend_forms::Bool=fals
n = 1
for op in userefs(stmt)
op = op[]
check_op(ir, domtree, op, bb, idx, print, isforeigncall, n, allow_frontend_forms)
check_op(ir, domtree, op, bb, idx, idx, print, isforeigncall, n, allow_frontend_forms)
n += 1
end
end
Expand Down
15 changes: 15 additions & 0 deletions test/compiler/irpasses.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,21 @@ let m = Meta.@lower 1 + 1
@test length(ir.cfg.blocks) == 1
end

# `cfg_simplify!` shouldn't error in a presence of `try/catch` block
let ir = Base.code_ircode(; optimize_until="slot2ssa") do
v = try
catch
end
v
end |> only |> first
Core.Compiler.verify_ir(ir)
nb = length(ir.cfg.blocks)
ir = Core.Compiler.cfg_simplify!(ir)
Core.Compiler.verify_ir(ir)
na = length(ir.cfg.blocks)
@test na < nb
end

# Issue #29213
function f_29213()
while true
Expand Down

0 comments on commit ecf9e56

Please sign in to comment.