Skip to content

Commit

Permalink
reland "Inlining: Remove outdated code path for GlobalRef movement (#…
Browse files Browse the repository at this point in the history
…46880)"

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 #46951 due to
bugs like #46940 and #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.
  • Loading branch information
aviatesk committed Nov 1, 2024
1 parent 706a4f6 commit 7f9d042
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 14 deletions.
12 changes: 0 additions & 12 deletions base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -663,18 +663,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)
Expand Down
4 changes: 2 additions & 2 deletions test/compiler/EscapeAnalysis/EscapeAnalysis.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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{Base.RefValue{String}}()
$code_escapes((Base.RefValue{String},)) do s
Rx[] = s
Core.sizeof(Rx[])
end
Expand Down
7 changes: 7 additions & 0 deletions test/compiler/inline.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 7f9d042

Please sign in to comment.