From c0f71e7dbb51b1f0a58359d404e9c559858f60b7 Mon Sep 17 00:00:00 2001
From: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Date: Fri, 7 Jun 2024 11:14:08 +0900
Subject: [PATCH] inference: follow up #54323, override ssaflags with new cycle
 effects (#54689)

JuliaLang/julia#54323 ensures that all frames within a cycle have the
same, cycle valid effects. However, `src.ssaflags` is calculated using
partial effects, so when the effects of a `frame` within the cycle are
updated, there would be an inconsistency between `frame.ipo_effects` and
`frame.src.ssaflags`. Due to this inconsistency, JuliaLang/julia#54323
breaks the test cases from JuliaLang/julia#51092, when backported to
v1.11. On the surface this is because JuliaLang/julia#52999 hasn't been
backported to v1.11, but the fundamental issue lies in this
inconsistency between cycle effects and `ssaflags`.

To resolve this issue, this commit traverses `cycle_backedges` to visit
statements involved in the cycle, and updates each `ssaflags` according
to new cycle valid effects if necessary.
---
 base/compiler/typeinfer.jl | 54 +++++++++++++++++++++++++++++++++-----
 test/compiler/inference.jl | 11 +++++++-
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/base/compiler/typeinfer.jl b/base/compiler/typeinfer.jl
index fd45791b64a40..8b049efa241e7 100644
--- a/base/compiler/typeinfer.jl
+++ b/base/compiler/typeinfer.jl
@@ -244,9 +244,36 @@ function _typeinf(interp::AbstractInterpreter, frame::InferenceState)
     typeinf_nocycle(interp, frame) || return false # frame is now part of a higher cycle
     # with no active ip's, frame is done
     frames = frame.callers_in_cycle
-    isempty(frames) && push!(frames, frame)
+    if isempty(frames)
+        push!(frames, frame)
+        finish_nocycle(interp, frame)
+    elseif length(frames) == 1
+        @assert frames[1] === frame "invalid callers_in_cycle"
+        finish_nocycle(interp, frame)
+    else
+        finish_cycle(interp, frames)
+    end
+    empty!(frames)
+    return true
+end
+
+function finish_nocycle(::AbstractInterpreter, frame::InferenceState)
+    frame.dont_work_on_me = true
+    finish(frame, frame.interp)
+    opt = frame.result.src
+    if opt isa OptimizationState # implies `may_optimize(caller.interp) === true`
+        optimize(frame.interp, opt, frame.result)
+    end
+    finish!(frame.interp, frame)
+    if is_cached(frame)
+        cache_result!(frame.interp, frame.result)
+    end
+    return nothing
+end
+
+function finish_cycle(::AbstractInterpreter, frames::Vector{InferenceState})
     cycle_valid_worlds = WorldRange()
-    cycle_effects = EFFECTS_TOTAL
+    cycle_valid_effects = EFFECTS_TOTAL
     for caller in frames
         @assert !(caller.dont_work_on_me)
         caller.dont_work_on_me = true
@@ -255,11 +282,10 @@ function _typeinf(interp::AbstractInterpreter, frame::InferenceState)
         # that are simply the intersection of each partial computation, without having
         # dependencies on each other (unlike rt and exct)
         cycle_valid_worlds = intersect(cycle_valid_worlds, caller.valid_worlds)
-        cycle_effects = merge_effects(cycle_effects, caller.ipo_effects)
+        cycle_valid_effects = merge_effects(cycle_valid_effects, caller.ipo_effects)
     end
     for caller in frames
-        caller.valid_worlds = cycle_valid_worlds
-        caller.ipo_effects = cycle_effects
+        adjust_cycle_frame!(caller, cycle_valid_worlds, cycle_valid_effects)
         finish(caller, caller.interp)
     end
     for caller in frames
@@ -274,8 +300,22 @@ function _typeinf(interp::AbstractInterpreter, frame::InferenceState)
             cache_result!(caller.interp, caller.result)
         end
     end
-    empty!(frames)
-    return true
+    return nothing
+end
+
+function adjust_cycle_frame!(sv::InferenceState, cycle_valid_worlds::WorldRange, cycle_valid_effects::Effects)
+    sv.valid_worlds = cycle_valid_worlds
+    sv.ipo_effects = cycle_valid_effects
+    # traverse the callees of this cycle that are tracked within `sv.cycle_backedges`
+    # and adjust their statements so that they are consistent with the new `cycle_valid_effects`
+    new_flags = flags_for_effects(cycle_valid_effects)
+    for (callee, pc) in sv.cycle_backedges
+        old_currpc = callee.currpc
+        callee.currpc = pc
+        set_curr_ssaflag!(callee, new_flags, IR_FLAGS_EFFECTS)
+        callee.currpc = old_currpc
+    end
+    return nothing
 end
 
 function is_result_constabi_eligible(result::InferenceResult)
diff --git a/test/compiler/inference.jl b/test/compiler/inference.jl
index d560d4362068f..b661fcc384268 100644
--- a/test/compiler/inference.jl
+++ b/test/compiler/inference.jl
@@ -5290,7 +5290,16 @@ end
     return r
 end
 foo51090(b) = return bar51090(b)
-@test_broken !fully_eliminated(foo51090, (Int,))
+@test !fully_eliminated(foo51090, (Int,))
+
+Base.@assume_effects :terminates_globally @noinline function bar51090_terminates(b)
+    b == 0 && return
+    r = foo51090_terminates(b - 1)
+    Base.donotdelete(b)
+    return r
+end
+foo51090_terminates(b) = return bar51090_terminates(b)
+@test !fully_eliminated(foo51090_terminates, (Int,))
 
 # exploit throwness from concrete eval for intrinsics
 @test Base.return_types() do