Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

post-opt: add more test cases for visit_conditional_successors #53642

Merged
merged 1 commit into from
Mar 19, 2024

Commits on Mar 19, 2024

  1. post-opt: add more test cases for visit_conditional_successors

    This commit was initially crafted to fix an issue with post-opt analysis
    that came to light in #53613, planning to solve it by
    incorporating an augmented CFG directly into the post-opt analysis. But,
    with the issue now sorted out by #53739, this commit
    shifted focus to just adding test cases.
    
    === The original commit message ===
    
    post-opt: use augmented post-domtree for `visit_conditional_successors`
    
    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
    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 (#53613 (comment)).
    aviatesk committed Mar 19, 2024
    Configuration menu
    Copy the full SHA
    3fecda9 View commit details
    Browse the repository at this point in the history