post-opt: add more test cases for visit_conditional_successors

[aviatesk]

This commit fixes the first problem that was found while digging into #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> 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. Since the augmented post domtree is 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 (#53613 (comment)).

[topolarity]

Since the augmented domtree still does not have a unique exit (must-throw blocks are not attached to the exit node), I think this is still leaving a bug in place:

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])
           postdomtree = CC.construct_postdomtree(construct_augmented_cfg(ir).blocks)
           [CC.postdominates(postdomtree, bb, 1) for bb in 1:3]
       end
3-element Vector{Bool}:
 1
 1
 0

This seems to be under the impression that BB (2) post-dominates (1), which is clearly false. This is probably computing post-dominance in each exit branch under the assumption that the corresponding exit is ultimately taken (e.g. for the "nothrow" augmented code path, this is now reporting postdominance under a :nothrow assumption)

I think the fix is needed in the DomTree instead, so that the reported post-dominance corresponds to a must-happen-after relation like we'd expect.

[aviatesk]

This seems to be under the impression that BB (2) post-dominates (1), which is clearly false.

I'm not sure whether to call this a mistake or not. If you don't consider the unreachable node as an exit point, then the only real exit point becomes BB (2), which means you could say BB (2) post-dominates BB (1). The issue seems to be that when you build a post-dominance tree using the original, unaugmented CFG, you end up with results that suggest the opposite:

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])
           postdomtree = CC.construct_postdomtree(ir.cfg.blocks)
           [CC.postdominates(postdomtree, bb, 1) for bb in 1:3]
       end
3-element Vector{Bool}:
 1
 0
 1

So, I'm with you on the need for a fix on the DomTree side of things. We ought to get the right answers even if we don't use CC.construct_augumented_cfg in my opinion.

[topolarity]

I'm with you there, but do we know enough about the DomTree bug to know that this behaves correctly even though we still don't have a post-dominating exit node (from the POV of the DomTree algorithm)?

[topolarity]

Here's an example where this still seems to go wrong:

julia> let code = Any[
               # block 1 (we should visit this block)
               #= %1 =# Expr(:call, :opaque), # maybe-throw
               #= %2 =# GotoIfNot(Argument(2), 6),
               # block 2
               #= %3 =# GotoIfNot(Argument(3), 7),
               # block 3 (we should visit this block)
               #= %4 =# Expr(:call, throw, "potential throw"),
               #= %5 =# ReturnNode(), # unreachable
               # block 4
               #= %6 =# GotoNode(1),
               # block 5
               #= %7 =# ReturnNode(nothing)
           ]
           ir = make_ircode(code; slottypes=Any[Any,Bool,Bool])
           lazy = CC.LazyAugmentedDomtrees(ir)
           visited = BitSet()
           Core.Compiler.visit_conditional_successors(lazy, ir, #=bb=#1) do succ::Int
               push!(visited, succ)
               return false
           end
           visited
       end
BitSet([4])

Whether this returns nothing or either of two different errors can depend on Argument(2), but the only block we explore is GotoNode(1)

[aviatesk]

Actually, what's happening here is correct. That's because visit_conditional_successors is designed to check out the "conditional" successors w.r.t. a given bb. If you look at the control graph of the code above, any code execution from BB (1) will reach BB (2), no matter what code path is taken, which means BB (2) isn't a "conditional successor". So, there's no need to visit BB (3), which is a successor of BB (2).

[aviatesk]

@Keno, do you think it's appropriate to always add a virtual edge, regardless of whether the actual return node is reachable? I believe this adjustment is necessary for visit_conditional_successors but I'm uncertain about its implications for the usage in iterated_dominance_frontier at L853.

[Keno]

I agree with @topolarity that the postdomination query is wrong in the example. A block "A", postdominates "B" if every path that passes through B must also pass through "A". Our notion of postdominance does ignore throws, but I for those purposes, the unreachable is just treated as a return. In this case, I still think that's the correct query to cut off the walk.

[aviatesk]

So, are we saying that postdominates should automatically make use of the augmented domtree? Also, just to clarify, the behavior of the code shared by Cody above is right. The reason is, visit_conditional_successors(..., bb) isn't meant to visit successors that post-dominate bb. It's actually supposed to visit the conditional ones that don't postdominate it, which means visiting only BB (4) is the expected behavior.

[aviatesk]

@nanosoldier runbenchmarks("inference", vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.