From 715eb1dd51ee60cec30425affe5d14941a60d0ac Mon Sep 17 00:00:00 2001
From: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Date: Sat, 2 Nov 2024 15:27:10 +0900
Subject: [PATCH] reland "Inlining: Remove outdated code path for GlobalRef
 movement (#46880)" (#56382)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

From the description of the original PR:
> We used to not allow `GlobalRef` in `PhiNode` at all (because they
> could have side effects). However, we then change the IR to make
> side-effecting `GlobalRef`s illegal in statement position in general,
> so now `PhiNode`s values are just regular value position, so there's
> no reason any more to try to move `GlobalRef`s out to statement
> position in inlining. Moreover, doing so introduces a bunch of
> unnecessary `GlobalRef`s that weren't being moved back. We could fix
> that separately by setting appropriate flags, but it's simpler to just
> get rid of this special case entirely.

This change itself does not sound to have any issues, and in fact, it is
very useful for keeping the IR slim, especially in code generated by
Cassette-like systems, so I would like to reland it.

However, the original PR was reverted in JuliaLang/julia#46951 due to
bugs like JuliaLang/julia#46940 and JuliaLang/julia#46943. I could not
reproduce these bugs on my end (maybe they have been fixed on some
GC-side fixes?), so I believe relanding the original PR’s changes would
not cause any issues, but it is necessary to confirm that similar
problems do not arise before merging this PR.
---
 base/compiler/ssair/inlining.jl                | 12 ------------
 test/compiler/EscapeAnalysis/EscapeAnalysis.jl |  4 ++--
 test/compiler/inline.jl                        |  7 +++++++
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl
index 8d5ba8353b2c0..98be475520f01 100644
--- a/base/compiler/ssair/inlining.jl
+++ b/base/compiler/ssair/inlining.jl
@@ -665,18 +665,6 @@ function batch_inline!(ir::IRCode, todo::Vector{Pair{Int,Any}}, propagate_inboun
                     compact.active_result_bb -= 1
                     refinish = true
                 end
-                # It is possible for GlobalRefs and Exprs to be in argument position
-                # at this point in the IR, though in that case they are required
-                # to be effect-free. However, we must still move them out of argument
-                # position, since `Argument` is allowed in PhiNodes, but `GlobalRef`
-                # and `Expr` are not, so a substitution could anger the verifier.
-                for aidx in 1:length(argexprs)
-                    aexpr = argexprs[aidx]
-                    if isa(aexpr, Expr) || isa(aexpr, GlobalRef)
-                        ninst = removable_if_unused(NewInstruction(aexpr, argextype(aexpr, compact), compact.result[idx][:line]))
-                        argexprs[aidx] = insert_node_here!(compact, ninst)
-                    end
-                end
                 if isa(item, InliningTodo)
                     compact.ssa_rename[old_idx] = ir_inline_item!(compact, idx, argexprs, item, boundscheck, state.todo_bbs)
                 elseif isa(item, UnionSplit)
diff --git a/test/compiler/EscapeAnalysis/EscapeAnalysis.jl b/test/compiler/EscapeAnalysis/EscapeAnalysis.jl
index 9afe49c01562d..4799fe4cee5ca 100644
--- a/test/compiler/EscapeAnalysis/EscapeAnalysis.jl
+++ b/test/compiler/EscapeAnalysis/EscapeAnalysis.jl
@@ -671,8 +671,8 @@ end
         @test has_all_escape(result.state[Argument(3)]) # b
     end
     let result = @eval EATModule() begin
-            const Rx = SafeRef{String}("Rx")
-            $code_escapes((String,)) do s
+            const Rx = SafeRef(Ref(""))
+            $code_escapes((Base.RefValue{String},)) do s
                 Rx[] = s
                 Core.sizeof(Rx[])
             end
diff --git a/test/compiler/inline.jl b/test/compiler/inline.jl
index 53f7adc2a2a77..416f3873c5422 100644
--- a/test/compiler/inline.jl
+++ b/test/compiler/inline.jl
@@ -2282,3 +2282,10 @@ let;Base.Experimental.@force_compile
     f_EA_finalizer(42000)
     @test foreign_buffer_checker.finalized
 end
+
+# Test that inlining doesn't unnecessarily move things to statement position
+@noinline f_noinline_invoke(x::Union{Symbol,Nothing}=nothing) = Core.donotdelete(x)
+g_noinline_invoke(x) = f_noinline_invoke(x)
+let src = code_typed1(g_noinline_invoke, (Union{Symbol,Nothing},))
+    @test !any(@nospecialize(x)->isa(x,GlobalRef), src.code)
+end