Skip to content

Commit d6bbf85

Browse files
committed
post-opt analysis: fix conditional successor visitation logic (#53518)
Previously `conditional_successors_may_throw` performed post-domination analysis not on the initially specified `bb` (which was given as the argument), but on those BBs being analyzed that were popped from the work-queue at the time. As a result, there were cases where not all conditional successors were visited. fixes #53508
1 parent 7ef180e commit d6bbf85

File tree

3 files changed

+48
-7
lines changed

3 files changed

+48
-7
lines changed

base/compiler/optimize.jl

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -534,17 +534,22 @@ function any_stmt_may_throw(ir::IRCode, bb::Int)
534534
return false
535535
end
536536

537-
function conditional_successors_may_throw(lazypostdomtree::LazyPostDomtree, ir::IRCode, bb::Int)
537+
function visit_conditional_successors(callback, lazypostdomtree::LazyPostDomtree, ir::IRCode, bb::Int)
538538
visited = BitSet((bb,))
539539
worklist = Int[bb]
540540
while !isempty(worklist)
541-
thisbb = pop!(worklist)
541+
thisbb = popfirst!(worklist)
542542
for succ in ir.cfg.blocks[thisbb].succs
543543
succ in visited && continue
544544
push!(visited, succ)
545-
postdominates(get!(lazypostdomtree), succ, thisbb) && continue
546-
any_stmt_may_throw(ir, succ) && return true
547-
push!(worklist, succ)
545+
if postdominates(get!(lazypostdomtree), succ, bb)
546+
# this successor is not conditional, so no need to visit it further
547+
continue
548+
elseif callback(succ)
549+
return true
550+
else
551+
push!(worklist, succ)
552+
end
548553
end
549554
end
550555
return false
@@ -836,8 +841,10 @@ function ((; sv)::ScanStmt)(inst::Instruction, lstmt::Int, bb::Int)
836841
# inconsistent region.
837842
if !sv.result.ipo_effects.terminates
838843
sv.all_retpaths_consistent = false
839-
elseif conditional_successors_may_throw(sv.lazypostdomtree, sv.ir, bb)
840-
# Check if there are potential throws that require
844+
elseif visit_conditional_successors(sv.lazypostdomtree, sv.ir, bb) do succ::Int
845+
return any_stmt_may_throw(sv.ir, succ)
846+
end
847+
# check if this `GotoIfNot` leads to conditional throws, which taints consistency
841848
sv.all_retpaths_consistent = false
842849
else
843850
(; cfg, domtree) = get!(sv.lazyagdomtree)

test/compiler/effects.jl

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1382,3 +1382,8 @@ let; Base.Experimental.@force_compile; func52843(); end
13821382
# pointerref nothrow for invalid pointer
13831383
@test !Core.Compiler.intrinsic_nothrow(Core.Intrinsics.pointerref, Any[Type{Ptr{Vector{Int64}}}, Int, Int])
13841384
@test !Core.Compiler.intrinsic_nothrow(Core.Intrinsics.pointerref, Any[Type{Ptr{T}} where T, Int, Int])
1385+
1386+
# post-opt :consistent-cy analysis correctness
1387+
# https://github.com/JuliaLang/julia/issues/53508
1388+
@test !Core.Compiler.is_consistent(Base.infer_effects(getindex, (UnitRange{Int},Int)))
1389+
@test !Core.Compiler.is_consistent(Base.infer_effects(getindex, (Base.OneTo{Int},Int)))

test/compiler/ssair.jl

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,35 @@ let ci = code_lowered(()->@isdefined(_not_def_37919_), ())[1]
238238
@test Core.Compiler.verify_ir(ir) === nothing
239239
end
240240

241+
let code = Any[
242+
# block 1
243+
GotoIfNot(Argument(2), 4),
244+
# block 2
245+
GotoNode(3),
246+
# block 3
247+
Expr(:call, throw, "potential throw"),
248+
# block 4
249+
Expr(:call, Core.Intrinsics.add_int, Argument(3), Argument(4)),
250+
GotoNode(6),
251+
# block 5
252+
ReturnNode(SSAValue(4))
253+
]
254+
ir = make_ircode(code; slottypes=Any[Any,Bool,Int,Int])
255+
lazypostdomtree = Core.Compiler.LazyPostDomtree(ir)
256+
visited = BitSet()
257+
@test !Core.Compiler.visit_conditional_successors(lazypostdomtree, ir, #=bb=#1) do succ::Int
258+
push!(visited, succ)
259+
return false
260+
end
261+
@test 2 visited
262+
@test 3 visited
263+
@test 4 visited
264+
@test 5 visited
265+
oc = Core.OpaqueClosure(ir)
266+
@test oc(false, 1, 1) == 2
267+
@test_throws "potential throw" oc(true, 1, 1)
268+
end
269+
241270
# Test dynamic update of domtree with edge insertions and deletions in the
242271
# following CFG:
243272
#

0 commit comments

Comments
 (0)