From 96866cb8f5c8f28d96c2b9e4eb1ec4f3a00a705b Mon Sep 17 00:00:00 2001
From: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Date: Thu, 25 Apr 2024 05:18:52 +0900
Subject: [PATCH] post-opt: taint :consistent when statement may raise
 inconsistently (#54219)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When an inconsistent statement doesn’t affect the return value, post-opt
analysis will try to refine it to `:consistent`. However this refinement
is invalid if the statement could throw, as `:consistent` requires
consistent termination.

For the time being, this commit implements the most conservative fix.
There might be a need to analyze `:nothrow` in a data-flow sensitive way
in the post-opt analysis as like we do for `:consistent`.

- closes #53613
---
 base/compiler/optimize.jl | 53 ++++++++++++++++++++++-----------------
 test/compiler/effects.jl  |  7 ++++--
 2 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/base/compiler/optimize.jl b/base/compiler/optimize.jl
index 1428d4b5ec34f..115085591667d 100644
--- a/base/compiler/optimize.jl
+++ b/base/compiler/optimize.jl
@@ -830,31 +830,38 @@ function ((; sv)::ScanStmt)(inst::Instruction, lstmt::Int, bb::Int)
 
     stmt_inconsistent = scan_inconsistency!(inst, sv)
 
-    if stmt_inconsistent && inst.idx == lstmt
-        if isa(stmt, ReturnNode) && isdefined(stmt, :val)
+    if stmt_inconsistent
+        if !has_flag(inst[:flag], IR_FLAG_NOTHROW)
+            # Taint :consistent if this statement may raise since :consistent requires
+            # consistent termination. TODO: Separate :consistent_return and :consistent_termination from :consistent.
             sv.all_retpaths_consistent = false
-        elseif isa(stmt, GotoIfNot)
-            # Conditional Branch with inconsistent condition.
-            # If we do not know this function terminates, taint consistency, now,
-            # :consistent requires consistent termination. TODO: Just look at the
-            # 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
-                       return any_stmt_may_throw(sv.ir, succ)
-                   end
-                # check if this `GotoIfNot` leads to conditional throws, which taints consistency
+        end
+        if inst.idx == lstmt
+            if isa(stmt, ReturnNode) && isdefined(stmt, :val)
                 sv.all_retpaths_consistent = false
-            else
-                (; cfg, domtree) = get!(sv.lazyagdomtree)
-                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
-                        # return. TODO: Check if all the retvals are egal.
-                        sv.all_retpaths_consistent = false
-                    else
-                        visit_bb_phis!(sv.ir, succ) do phiidx::Int
-                            push!(sv.inconsistent, phiidx)
+            elseif isa(stmt, GotoIfNot)
+                # Conditional Branch with inconsistent condition.
+                # If we do not know this function terminates, taint consistency, now,
+                # :consistent requires consistent termination. TODO: Just look at the
+                # 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
+                        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)
+                    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
+                            # return. TODO: Check if all the retvals are egal.
+                            sv.all_retpaths_consistent = false
+                        else
+                            visit_bb_phis!(sv.ir, succ) do phiidx::Int
+                                push!(sv.inconsistent, phiidx)
+                            end
                         end
                     end
                 end
diff --git a/test/compiler/effects.jl b/test/compiler/effects.jl
index d3884bf2c73e2..36c28b6c844bf 100644
--- a/test/compiler/effects.jl
+++ b/test/compiler/effects.jl
@@ -1102,7 +1102,7 @@ function f3_optrefine(x)
     @fastmath sqrt(x)
     return x
 end
-@test Core.Compiler.is_consistent(Base.infer_effects(f3_optrefine))
+@test Core.Compiler.is_consistent(Base.infer_effects(f3_optrefine, (Float64,)))
 
 # Check that :consistent is properly modeled for throwing statements
 const GLOBAL_MUTABLE_SWITCH = Ref{Bool}(false)
@@ -1390,10 +1390,13 @@ let; Base.Experimental.@force_compile; func52843(); end
 
 @noinline f53613() = @assert isdefined(@__MODULE__, :v53613)
 g53613() = f53613()
+h53613() = g53613()
 @test !Core.Compiler.is_consistent(Base.infer_effects(f53613))
-@test_broken !Core.Compiler.is_consistent(Base.infer_effects(g53613))
+@test !Core.Compiler.is_consistent(Base.infer_effects(g53613))
 @test_throws AssertionError f53613()
 @test_throws AssertionError g53613()
+@test_throws AssertionError h53613()
 global v53613 = nothing
 @test f53613() === nothing
 @test g53613() === nothing
+@test h53613() === nothing