From 2fc4e2d61155adbde10fcfa3dd8e465bdf780ed7 Mon Sep 17 00:00:00 2001
From: Keno Fischer <keno@juliacomputing.com>
Date: Fri, 26 Apr 2024 15:15:00 -0400
Subject: [PATCH] cfg_simplify: Don't accidentally consider `GotoIfNot` a
 throwing terminator (#54262)

This addresses a bug in #54216, where a `GotoIfNot` was accidentally
considered a throwing terminator. Just as I was about to PR this, I
noticed that #54260 had already been opened for the same issue. However,
there's three differences in this PR that made me open it anyway:

1. There's an additional missing case where the terminator is `nothing`
which has special case semantics of allowing any type on it, because it
serves as a deletion marker.

2. My test also test the `EnterNode` and `:leave` terminators, just to
have a complete sampling.

3. I like the code flow in this version slightly better.

Co-authored-by: Cody Tapscott <cody.tapscott@juliahub.com>
---
 base/compiler/ssair/irinterp.jl |  2 +-
 base/compiler/ssair/passes.jl   | 22 +++++++++++++------
 test/compiler/irpasses.jl       | 38 +++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/base/compiler/ssair/irinterp.jl b/base/compiler/ssair/irinterp.jl
index 05d78e8706072..3a0d8c6c52f00 100644
--- a/base/compiler/ssair/irinterp.jl
+++ b/base/compiler/ssair/irinterp.jl
@@ -325,7 +325,7 @@ function _ir_abstract_constant_propagation(interp::AbstractInterpreter, irsv::IR
             delete!(ssa_refined, idx)
         end
         check_ret!(stmt, idx)
-        is_terminator_or_phi = (isa(stmt, PhiNode) || isterminator(stmt))
+        is_terminator_or_phi = (isa(stmt, PhiNode) || stmt === nothing || isterminator(stmt))
         if typ === Bottom && !(idx == lstmt && is_terminator_or_phi)
             return true
         end
diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl
index 34f7ed4a21cb0..8426611c3df37 100644
--- a/base/compiler/ssair/passes.jl
+++ b/base/compiler/ssair/passes.jl
@@ -2341,12 +2341,8 @@ function cfg_simplify!(ir::IRCode)
                     curr = merged_succ[curr]
                 end
                 terminator = ir[SSAValue(bbs[curr].stmts[end])][:stmt]
-                is_throw = ir[SSAValue(bbs[curr].stmts[end])][:type] === Union{} && !isa(terminator, PhiNode)
-                if isa(terminator, GotoNode) || isa(terminator, ReturnNode) || is_throw
-                    # Only advance to next block if it's a successor
-                    # (avoid GotoNode, ReturnNode, throw()/Union{})
-                    break
-                elseif isa(terminator, GotoIfNot)
+
+                if isa(terminator, GotoIfNot)
                     if bb_rename_succ[terminator.dest] == 0
                         push!(worklist, terminator.dest)
                     end
@@ -2355,6 +2351,20 @@ function cfg_simplify!(ir::IRCode)
                     if bb_rename_succ[catchbb] == 0
                         push!(worklist, catchbb)
                     end
+                elseif isa(terminator, GotoNode) || isa(terminator, ReturnNode)
+                    # No implicit fall through. Schedule from work list.
+                    break
+                else
+                    is_bottom = ir[SSAValue(bbs[curr].stmts[end])][:type] === Union{}
+                    if is_bottom && !isa(terminator, PhiNode) && terminator !== nothing
+                        # If this is a regular statement (not PhiNode/GotoNode/GotoIfNot
+                        # or the `nothing` special case deletion marker),
+                        # and the type is Union{}, then this may be a terminator.
+                        # Ordinarily we normalize with ReturnNode(), but this is not
+                        # required. In any case, we do not fall through, so we
+                        # do not need to schedule the fall-through block.
+                        break
+                    end
                 end
                 ncurr = curr + 1
                 while !isempty(searchsorted(dropped_bbs, ncurr))
diff --git a/test/compiler/irpasses.jl b/test/compiler/irpasses.jl
index 21bf0a59fd289..5c56b6eb76aa5 100644
--- a/test/compiler/irpasses.jl
+++ b/test/compiler/irpasses.jl
@@ -1911,3 +1911,41 @@ let code = Any[
     Core.Compiler.verify_ir(ir)
     @test length(ir.cfg.blocks) == 3 # should have removed block 3
 end
+
+let code = Any[
+        # block 1
+        EnterNode(4, 1),
+        # block 2
+        GotoNode(3), # will be turned into nothing
+        # block 3
+        GotoNode(5),
+        # block 4
+        ReturnNode(),
+        # block 5
+        Expr(:leave, SSAValue(1)),
+        # block 6
+        GotoIfNot(Core.Argument(1), 8),
+        # block 7
+        ReturnNode(1),
+        # block 8
+        ReturnNode(2),
+    ]
+    ir = make_ircode(code; ssavaluetypes=Any[Union{}, Union{}, Union{}, Union{}, Nothing, Union{}, Union{}, Union{}])
+    @test length(ir.cfg.blocks) == 8
+    Core.Compiler.verify_ir(ir)
+
+    # Union typed deletion marker in basic block 2
+    Core.Compiler.setindex!(ir, nothing, SSAValue(2))
+
+    # Test cfg_simplify
+    Core.Compiler.verify_ir(ir)
+    ir = Core.Compiler.cfg_simplify!(ir)
+    Core.Compiler.verify_ir(ir)
+    @test length(ir.cfg.blocks) == 6
+    gotoifnot = Core.Compiler.last(ir.cfg.blocks[3].stmts)
+    inst = ir[SSAValue(gotoifnot)]
+    @test isa(inst[:stmt], GotoIfNot)
+    # Make sure we didn't accidentally schedule the unreachable block as
+    # fallthrough
+    @test isdefined(ir[SSAValue(gotoifnot+1)][:inst]::ReturnNode, :val)
+end