From ecf9e56037f3b6fd6a27a30a300806343c8ec033 Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Thu, 20 Oct 2022 15:04:29 -0400 Subject: [PATCH] cfg_simplify: Fix more bugs (#47240) 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. --- base/compiler/ssair/ir.jl | 2 +- base/compiler/ssair/passes.jl | 36 +++++++++++++-- base/compiler/ssair/slot2ssa.jl | 10 ++--- base/compiler/ssair/verify.jl | 77 ++++++++++++++++++++++----------- test/compiler/irpasses.jl | 15 +++++++ 5 files changed, 106 insertions(+), 34 deletions(-) diff --git a/base/compiler/ssair/ir.jl b/base/compiler/ssair/ir.jl index f5867a9a1a988..7fee3ccd29112 100644 --- a/base/compiler/ssair/ir.jl +++ b/base/compiler/ssair/ir.jl @@ -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) diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl index f0542d67f8f38..8b9251f9e369b 100644 --- a/base/compiler/ssair/passes.jl +++ b/base/compiler/ssair/passes.jl @@ -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 @@ -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)) @@ -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 @@ -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 @@ -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)) @@ -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 diff --git a/base/compiler/ssair/slot2ssa.jl b/base/compiler/ssair/slot2ssa.jl index 694333156753a..4197bcda34a44 100644 --- a/base/compiler/ssair/slot2ssa.jl +++ b/base/compiler/ssair/slot2ssa.jl @@ -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) diff --git a/base/compiler/ssair/verify.jl b/base/compiler/ssair/verify.jl index d08c187ea2a7e..04e6ac643ac67 100644 --- a/base/compiler/ssair/verify.jl +++ b/base/compiler/ssair/verify.jl @@ -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) @@ -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 @@ -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" @@ -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" @@ -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 @@ -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] @@ -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) @@ -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 @@ -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 diff --git a/test/compiler/irpasses.jl b/test/compiler/irpasses.jl index ec26cab55da14..0de0563cfb846 100644 --- a/test/compiler/irpasses.jl +++ b/test/compiler/irpasses.jl @@ -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