From 9928c451b53f0083844e10665edb66f680c2dae0 Mon Sep 17 00:00:00 2001 From: Shuhei Kadowaki Date: Fri, 8 Mar 2024 02:28:02 +0900 Subject: [PATCH] post-opt: use augmented post-domtree for `visit_conditional_successors` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes the first problem that was found while digging into JuliaLang/julia#53613. It turns out that the post-domtree constructed from regular `IRCode` doesn't work for visiting conditional successors for post-opt analysis in cases like: ```julia julia> let code = Any[ # block 1 GotoIfNot(Argument(2), 3), # block 2 ReturnNode(Argument(3)), # block 3 (we should visit this block) Expr(:call, throw, "potential throw"), ReturnNode(), # unreachable ] ir = make_ircode(code; slottypes=Any[Any,Bool,Bool]) visited = BitSet() @test !Core.Compiler.visit_conditional_successors(CC.LazyPostDomtree(ir), ir, #=bb=#1) do succ::Int push!(visited, succ) return false end @test 2 ∉ visited @test 3 ∈ visited end Test Failed at REPL[14]:16 Expression: 2 ∉ visited Evaluated: 2 ∉ BitSet([2]) ``` This might mean that we need to fix on the `postdominates` end, but for now, this commit tries to get around it by using the augmented post domtree in `visit_conditional_successors`, while also enforcing the augmented control flow graph (`construct_augmented_cfg`) to have a single exit node really. Since the augmented post domtree is now enforced to have a single return, we can keep using the current `postdominates` to fix the issue. However, this commit isn't enough to fix the NeuralNetworkReachability segfault as reported in #53613, and we need to tackle the second issue reported there too (https://github.com/JuliaLang/julia/issues/53613#issuecomment-1983243419). --- base/compiler/optimize.jl | 81 ++++++++++++++++++++++----------------- test/compiler/effects.jl | 10 +++++ test/compiler/ssair.jl | 66 +++++++++++++++++++++++++------ 3 files changed, 110 insertions(+), 47 deletions(-) diff --git a/base/compiler/optimize.jl b/base/compiler/optimize.jl index 81b025e588f7f..92ea49d9595b9 100644 --- a/base/compiler/optimize.jl +++ b/base/compiler/optimize.jl @@ -527,7 +527,45 @@ function any_stmt_may_throw(ir::IRCode, bb::Int) return false end -function visit_conditional_successors(callback, lazypostdomtree::LazyPostDomtree, ir::IRCode, bb::Int) +mutable struct LazyAugmentedDomtrees + const ir::IRCode + cfg::CFG + domtree::DomTree + postdomtree::PostDomTree + LazyAugmentedDomtrees(ir::IRCode) = new(ir) +end + +function get!(lazyagdomtrees::LazyAugmentedDomtrees, sym::Symbol) + isdefined(lazyagdomtrees, sym) && return getfield(lazyagdomtrees, sym) + if sym === :cfg + return lazyagdomtrees.cfg = construct_augmented_cfg(lazyagdomtrees.ir) + elseif sym === :domtree + return lazyagdomtrees.domtree = construct_domtree(get!(lazyagdomtrees, :cfg)) + elseif sym === :postdomtree + return lazyagdomtrees.postdomtree = construct_postdomtree(get!(lazyagdomtrees, :cfg)) + else + error("invalid field access") + end +end + +function construct_augmented_cfg(ir::IRCode) + cfg = copy(ir.cfg) + # Add a virtual basic block to represent the single exit + push!(cfg.blocks, BasicBlock(StmtRange(0:-1))) + for bb = 1:(length(cfg.blocks)-1) + terminator = ir[SSAValue(last(cfg.blocks[bb].stmts))][:stmt] + if terminator isa ReturnNode + cfg_insert_edge!(cfg, bb, length(cfg.blocks)) + end + end + return cfg +end + +visit_conditional_successors(callback, ir::IRCode, bb::Int) = + visit_conditional_successors(callback, construct_postdomtree(construct_augmented_cfg(ir)), ir, bb) +visit_conditional_successors(callback, lazyagdomtrees::LazyAugmentedDomtrees, ir::IRCode, bb::Int) = + visit_conditional_successors(callback, get!(lazyagdomtrees, :postdomtree), ir, bb) +function visit_conditional_successors(callback, postdomtree::PostDomTree, ir::IRCode, bb::Int) visited = BitSet((bb,)) worklist = Int[bb] while !isempty(worklist) @@ -535,7 +573,7 @@ function visit_conditional_successors(callback, lazypostdomtree::LazyPostDomtree for succ in ir.cfg.blocks[thisbb].succs succ in visited && continue push!(visited, succ) - if postdominates(get!(lazypostdomtree), succ, bb) + if postdominates(postdomtree, succ, bb) # this successor is not conditional, so no need to visit it further continue elseif callback(succ) @@ -548,40 +586,12 @@ function visit_conditional_successors(callback, lazypostdomtree::LazyPostDomtree return false end -struct AugmentedDomtree - cfg::CFG - domtree::DomTree -end - -mutable struct LazyAugmentedDomtree - const ir::IRCode - agdomtree::AugmentedDomtree - LazyAugmentedDomtree(ir::IRCode) = new(ir) -end - -function get!(lazyagdomtree::LazyAugmentedDomtree) - isdefined(lazyagdomtree, :agdomtree) && return lazyagdomtree.agdomtree - ir = lazyagdomtree.ir - cfg = copy(ir.cfg) - # Add a virtual basic block to represent the exit - push!(cfg.blocks, BasicBlock(StmtRange(0:-1))) - for bb = 1:(length(cfg.blocks)-1) - terminator = ir[SSAValue(last(cfg.blocks[bb].stmts))][:stmt] - if isa(terminator, ReturnNode) && isdefined(terminator, :val) - cfg_insert_edge!(cfg, bb, length(cfg.blocks)) - end - end - domtree = construct_domtree(cfg) - return lazyagdomtree.agdomtree = AugmentedDomtree(cfg, domtree) -end - mutable struct PostOptAnalysisState const result::InferenceResult const ir::IRCode const inconsistent::BitSetBoundedMinPrioritySet const tpdum::TwoPhaseDefUseMap - const lazypostdomtree::LazyPostDomtree - const lazyagdomtree::LazyAugmentedDomtree + const lazyagdomtrees::LazyAugmentedDomtrees const ea_analysis_pending::Vector{Int} all_retpaths_consistent::Bool all_effect_free::Bool @@ -592,9 +602,8 @@ mutable struct PostOptAnalysisState function PostOptAnalysisState(result::InferenceResult, ir::IRCode) inconsistent = BitSetBoundedMinPrioritySet(length(ir.stmts)) tpdum = TwoPhaseDefUseMap(length(ir.stmts)) - lazypostdomtree = LazyPostDomtree(ir) - lazyagdomtree = LazyAugmentedDomtree(ir) - return new(result, ir, inconsistent, tpdum, lazypostdomtree, lazyagdomtree, Int[], + lazyagdomtrees = LazyAugmentedDomtrees(ir) + return new(result, ir, inconsistent, tpdum, lazyagdomtrees, Int[], true, true, nothing, true, true, false) end end @@ -834,13 +843,13 @@ function ((; sv)::ScanStmt)(inst::Instruction, lstmt::Int, bb::Int) # inconsistent region. if !sv.result.ipo_effects.terminates sv.all_retpaths_consistent = false - elseif visit_conditional_successors(sv.lazypostdomtree, sv.ir, bb) do succ::Int + elseif visit_conditional_successors(sv.lazyagdomtrees, sv.ir, bb) do succ::Int return any_stmt_may_throw(sv.ir, succ) end # check if this `GotoIfNot` leads to conditional throws, which taints consistency sv.all_retpaths_consistent = false else - (; cfg, domtree) = get!(sv.lazyagdomtree) + cfg, domtree = get!(sv.lazyagdomtrees, :cfg), get!(sv.lazyagdomtrees, :domtree) for succ in iterated_dominance_frontier(cfg, BlockLiveness(sv.ir.cfg.blocks[bb].succs, nothing), domtree) if succ == length(cfg.blocks) # Phi node in the virtual exit -> We have a conditional diff --git a/test/compiler/effects.jl b/test/compiler/effects.jl index fa70c8de9d853..cdf797fb2a77b 100644 --- a/test/compiler/effects.jl +++ b/test/compiler/effects.jl @@ -1387,3 +1387,13 @@ let; Base.Experimental.@force_compile; func52843(); end # https://github.com/JuliaLang/julia/issues/53508 @test !Core.Compiler.is_consistent(Base.infer_effects(getindex, (UnitRange{Int},Int))) @test !Core.Compiler.is_consistent(Base.infer_effects(getindex, (Base.OneTo{Int},Int))) + +@noinline f53613() = @assert isdefined(@__MODULE__, :v53613) +g53613() = f53613() +@test !Core.Compiler.is_consistent(Base.infer_effects(f53613)) +@test_broken !Core.Compiler.is_consistent(Base.infer_effects(g53613)) +@test_throws AssertionError f53613() +@test_throws AssertionError g53613() +global v53613 = nothing +@test f53613() === nothing +@test g53613() === nothing diff --git a/test/compiler/ssair.jl b/test/compiler/ssair.jl index 70a2ab4a3acd9..9082d1640e3be 100644 --- a/test/compiler/ssair.jl +++ b/test/compiler/ssair.jl @@ -233,35 +233,79 @@ let code = Any[ end # issue #37919 -let ci = code_lowered(()->@isdefined(_not_def_37919_), ())[1] +let ci = only(code_lowered(()->@isdefined(_not_def_37919_), ())) ir = Core.Compiler.inflate_ir(ci) @test Core.Compiler.verify_ir(ir) === nothing end let code = Any[ # block 1 - GotoIfNot(Argument(2), 4), + GotoIfNot(Argument(2), 4) # block 2 - GotoNode(3), + Expr(:call, throw, "potential throw") + ReturnNode() # unreachable # block 3 - Expr(:call, throw, "potential throw"), + ReturnNode(Argument(3)) + ] + ir = make_ircode(code; slottypes=Any[Any,Bool,Int]) + visited = BitSet() + @test !Core.Compiler.visit_conditional_successors(ir, #=bb=#1) do succ::Int + push!(visited, succ) + return false + end + @test 2 ∈ visited + @test 3 ∈ visited + oc = Core.OpaqueClosure(ir) + @test oc(false, 1) == 1 + @test_throws "potential throw" oc(true, 1) +end + +let code = Any[ + # block 1 + GotoIfNot(Argument(2), 3) + # block 2 + ReturnNode(Argument(3)) + # block 3 + Expr(:call, throw, "potential throw") + ReturnNode() # unreachable + ] + ir = make_ircode(code; slottypes=Any[Any,Bool,Int]) + visited = BitSet() + @test !Core.Compiler.visit_conditional_successors(ir, #=bb=#1) do succ::Int + push!(visited, succ) + return false + end + @test 2 ∈ visited + @test 3 ∈ visited + oc = Core.OpaqueClosure(ir) + @test oc(true, 1) == 1 + @test_throws "potential throw" oc(false, 1) +end + +let code = Any[ + # block 1 + GotoIfNot(Argument(2), 5) + # block 2 + GotoNode(3) + # block 3 + Expr(:call, throw, "potential throw") + ReturnNode() # block 4 - Expr(:call, Core.Intrinsics.add_int, Argument(3), Argument(4)), - GotoNode(6), + Expr(:call, Core.Intrinsics.add_int, Argument(3), Argument(4)) + GotoNode(7) # block 5 - ReturnNode(SSAValue(4)) + ReturnNode(SSAValue(5)) ] ir = make_ircode(code; slottypes=Any[Any,Bool,Int,Int]) - lazypostdomtree = Core.Compiler.LazyPostDomtree(ir) visited = BitSet() - @test !Core.Compiler.visit_conditional_successors(lazypostdomtree, ir, #=bb=#1) do succ::Int + @test !Core.Compiler.visit_conditional_successors(ir, #=bb=#1) do succ::Int push!(visited, succ) return false end @test 2 ∈ visited @test 3 ∈ visited - @test 4 ∉ visited - @test 5 ∉ visited + @test 4 ∈ visited + @test 5 ∈ visited oc = Core.OpaqueClosure(ir) @test oc(false, 1, 1) == 2 @test_throws "potential throw" oc(true, 1, 1)