Skip to content

WIP/RFC: Change lowering of GC.preserve to be lexical #59129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

WIP/RFC: Change lowering of GC.preserve to be lexical #59129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Compiler/src/abstractinterpretation.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2883,6 +2883,14 @@ function abstract_call_unknown(interp::AbstractInterpreter, @nospecialize(ft),
return Future(CallMeta(rewrap_unionall(uft.parameters[2], wft), Any, Effects(), NoCallInfo()))
end
return Future(CallMeta(Any, Any, Effects(), NoCallInfo()))
elseif hasintersect(wft, Core.GCPreserveDuring)
wrapped_ft = getfield_tfunc(typeinf_lattice(interp), ft, Const(1))
newargtypes = copy(arginfo.argtypes)
newargtypes[1] = wrapped_ft
call = abstract_call(interp, ArgInfo(nothing, newargtypes), si, sv, max_methods)
return Future{CallMeta}(call, interp, sv) do call, interp, sv
CallMeta(call.rt, call.exct, call.effects, GCPreserveDuringCallInfo(call.info))
end
end
# non-constant function, but the number of arguments is known and the `f` is not a builtin or intrinsic
atype = argtypes_to_type(arginfo.argtypes)
Expand Down Expand Up @@ -3158,6 +3166,10 @@ function abstract_eval_new(interp::AbstractInterpreter, e::Expr, sstate::Stateme
end
end
end
# Special case GCPreserveDuring's root type is not observable - don't try to const prop it
if rt === GCPreserveDuring
ats[2] = Any
end
rt = PartialStruct(𝕃ᵢ, rt, undefs, ats)
end
else
Expand Down
117 changes: 100 additions & 17 deletions Compiler/src/ssair/inlining.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@ function InliningTodo(mi::MethodInstance, ir::IRCode, spec_info::SpecInfo, di::D
return InliningTodo(mi, ir, spec_info, di, linear_inline_eligible(ir), effects)
end

struct GCPreserveRewrite
actual_callee::Any
preservee::Any
end

struct GCPreserveInliningTodo
gc_preserve::GCPreserveRewrite
inlining::InliningTodo
end

struct ConstantCase
val::Any
edge::CodeInstance
Expand Down Expand Up @@ -357,8 +367,31 @@ function adjust_boundscheck!(inline_compact::IncrementalCompact, idx′::Int, st
return nothing
end

function apply_gc_preserve_inlining!(insert_node!::Inserter, inline_compact::IncrementalCompact, line, @nospecialize(stmt′), @nospecialize(gc_preserve))
if isexpr(stmt′, :invoke_modify)
# TODO: The combination of an existing invoke_modify and gc_preserve causes trouble. Bail for now.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I wasn't particularly in favor of adding a different, unrelated meaning to invoke_modify, instead of just adding some other metadata representation. I am somewhat curious if we can make the initial GCPreserveDuring implementation here go fully to being a CallOperand builtin function wrapper.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean that the codegen for invoke would recognize it when doing a specfun call and automatically strip it? I think that's a reasonable resolution to the TODO also, but I was a little worried of what would happen if that type information got lost shomehow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think wrapping the first argument in a marker is a sensible way to get the distinction in, because it really is like you're just calling the method instance with the preserve semantics, except codegen inlines that to a jl_roots operand bundle. I think that could be quite generic of a mechanism.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that part seems to make sense. Another design thought though is whether your call preconditions proposal is in essence the same mechanism also?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That proposal actually versions the generated code though, where here we want to version the caller, but specifically not the generated code, so while there are some similarities, I think the mechanisms are complements of each other.

stmt′.head = :call
popfirst!(stmt′.args)
end
if isexpr(stmt′, :call) || isexpr(stmt′, :invoke)
refarg = stmt′.head === :call ? 1 : 2
if stmt′.head === :invoke
stmt′.head = :invoke_modify
end
oldarg = stmt′.args[refarg]
stmt′.args[refarg] = insert_node!(NewInstruction(
Expr(:new, GlobalRef(Core, :GCPreserveDuring), oldarg, gc_preserve),
PartialStruct(Core.GCPreserveDuring,
Any[argextype(oldarg, inline_compact), argextype(gc_preserve, inline_compact)]),
NoCallInfo(), line,
IR_FLAG_CONSISTENT | IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW))
elseif isexpr(stmt′, :foreigncall)
push!(stmt′.args, gc_preserve)
end
end

function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector{Any},
item::InliningTodo, boundscheck::Symbol, todo_bbs::Vector{Tuple{Int, Int}})
item::InliningTodo, boundscheck::Symbol, todo_bbs::Vector{Tuple{Int, Int}}, @nospecialize(gc_preserve::Any))
# Ok, do the inlining here
inlined_at = compact.result[idx][:line]
ssa_substitute = ir_prepare_inlining!(InsertHere(compact), compact, item.ir, item.spec_info, item.di, item.mi, inlined_at, argexprs)
Expand All @@ -380,7 +413,8 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
# something better eventually.
inline_compact[idx′] = nothing
# alter the line number information for InsertBefore to point to the current instruction in the new linetable
inline_compact[SSAValue(idx′)][:line] = (ssa_substitute.inlined_at[1], ssa_substitute.inlined_at[2], Int32(lineidx))
line = (ssa_substitute.inlined_at[1], ssa_substitute.inlined_at[2], Int32(lineidx))
inline_compact[SSAValue(idx′)][:line] = line
insert_node! = InsertBefore(inline_compact, SSAValue(idx′))
stmt′ = ssa_substitute_op!(insert_node!, inline_compact[SSAValue(idx′)], stmt′, ssa_substitute)
if isa(stmt′, ReturnNode)
Expand All @@ -394,6 +428,8 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
break
elseif isexpr(stmt′, :boundscheck)
adjust_boundscheck!(inline_compact, idx′, stmt′, boundscheck)
elseif gc_preserve !== nothing
apply_gc_preserve_inlining!(insert_node!, inline_compact, line, stmt′, gc_preserve)
end
inline_compact[idx′] = stmt′
end
Expand All @@ -412,7 +448,8 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
@assert isempty(inline_compact.perm) && isempty(inline_compact.pending_perm) "linetable not in canonical form (missing compact call)"
for ((lineidx, idx′), stmt′) in inline_compact
inline_compact[idx′] = nothing
inline_compact[SSAValue(idx′)][:line] = (ssa_substitute.inlined_at[1], ssa_substitute.inlined_at[2], Int32(lineidx))
line = (ssa_substitute.inlined_at[1], ssa_substitute.inlined_at[2], Int32(lineidx))
inline_compact[SSAValue(idx′)][:line] = line
insert_node! = InsertBefore(inline_compact, SSAValue(idx′))
stmt′ = ssa_substitute_op!(insert_node!, inline_compact[SSAValue(idx′)], stmt′, ssa_substitute)
if isa(stmt′, ReturnNode)
Expand All @@ -433,6 +470,8 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
stmt′ = PhiNode(Int32[edge+bb_offset for edge in stmt′.edges], stmt′.values)
elseif isexpr(stmt′, :boundscheck)
adjust_boundscheck!(inline_compact, idx′, stmt′, boundscheck)
elseif gc_preserve !== nothing
apply_gc_preserve_inlining!(insert_node!, inline_compact, line, stmt′, gc_preserve)
end
inline_compact[idx′] = stmt′
end
Expand Down Expand Up @@ -578,7 +617,7 @@ function ir_inline_unionsplit!(compact::IncrementalCompact, idx::Int, argexprs::
end
end
if isa(case, InliningTodo)
val = ir_inline_item!(compact, idx, argexprs′, case, boundscheck, todo_bbs)
val = ir_inline_item!(compact, idx, argexprs′, case, boundscheck, todo_bbs, nothing)
elseif isa(case, InvokeCase)
invoke_stmt = Expr(:invoke, case.invoke, argexprs′...)
flag = flags_for_effects(case.effects)
Expand Down Expand Up @@ -624,7 +663,19 @@ function batch_inline!(ir::IRCode, todo::Vector{Pair{Int,Any}}, propagate_inboun
if isa(item, UnionSplit)
cfg_inline_unionsplit!(ir, idx, item, state, params)
else
item = item::InliningTodo
if isa(item, GCPreserveInliningTodo)
item = item::GCPreserveInliningTodo
# Rewrite the call now to drop the GCPreserveDuring, since we're committed to inlining.
# This makes sure that it gets renamed properly. We also need to rename the GC preservee,
# but we don't have a good place to put that, so temporarily append it to the argument list -
# we'll undo this below.
stmt = ir[SSAValue(idx)][:stmt]
stmt.args[1] = item.gc_preserve.actual_callee
push!(stmt.args, item.gc_preserve.preservee)
item = item.inlining
else
item = item::InliningTodo
end
# A linear inline does not modify the CFG
item.linear_inline_eligible && continue
cfg_inline_item!(ir, idx, item, state, false)
Expand Down Expand Up @@ -660,7 +711,9 @@ function batch_inline!(ir::IRCode, todo::Vector{Pair{Int,Any}}, propagate_inboun
refinish = true
end
if isa(item, InliningTodo)
compact.ssa_rename[old_idx] = ir_inline_item!(compact, idx, argexprs, item, boundscheck, state.todo_bbs)
compact.ssa_rename[old_idx] = ir_inline_item!(compact, idx, argexprs, item, boundscheck, state.todo_bbs, nothing)
elseif isa(item, GCPreserveInliningTodo)
compact.ssa_rename[old_idx] = ir_inline_item!(compact, idx, argexprs, item.inlining, boundscheck, state.todo_bbs, pop!(argexprs))
elseif isa(item, UnionSplit)
compact.ssa_rename[old_idx] = ir_inline_unionsplit!(compact, idx, argexprs, item, boundscheck, state.todo_bbs, interp)
end
Expand Down Expand Up @@ -988,25 +1041,30 @@ function retrieve_ir_for_inlining(mi::MethodInstance, opt::OptimizationState, pr
end

function handle_single_case!(todo::Vector{Pair{Int,Any}},
ir::IRCode, idx::Int, stmt::Expr, @nospecialize(case),
ir::IRCode, idx::Int, stmt::Expr, @nospecialize(case), gc_preserve::Union{GCPreserveRewrite, Nothing},
isinvoke::Bool = false)
if isa(case, ConstantCase)
ir[SSAValue(idx)][:stmt] = case.val
elseif isa(case, InvokeCase)
is_foldable_nothrow(case.effects) && inline_const_if_inlineable!(ir[SSAValue(idx)]) && return nothing
isinvoke && rewrite_invoke_exprargs!(stmt)
if stmt.head === :invoke
if gc_preserve !== nothing
stmt.head = :invoke_modify
end
stmt.args[1] = case.invoke
else
stmt.head = :invoke
stmt.head = gc_preserve === nothing ? :invoke : :invoke_modify
pushfirst!(stmt.args, case.invoke)
end
add_flag!(ir[SSAValue(idx)], flags_for_effects(case.effects))
elseif case === nothing
# Do, well, nothing
else
isinvoke && rewrite_invoke_exprargs!(stmt)
push!(todo, idx=>(case::InliningTodo))
case = case::InliningTodo
push!(todo, idx=>gc_preserve === nothing ? case :
GCPreserveInliningTodo(gc_preserve, case))
end
return nothing
end
Expand Down Expand Up @@ -1237,6 +1295,7 @@ end
# functions.
function process_simple!(todo::Vector{Pair{Int,Any}}, ir::IRCode, idx::Int, flag::UInt32,
state::InliningState)
gc_preserve = nothing
inst = ir[SSAValue(idx)]
stmt = inst[:stmt]
if !(stmt isa Expr)
Expand All @@ -1262,6 +1321,22 @@ function process_simple!(todo::Vector{Pair{Int,Any}}, ir::IRCode, idx::Int, flag
sig = call_sig(ir, stmt)
sig === nothing && return nothing

# Handle GCPreserveDuring
if isa(inst[:info], GCPreserveDuringCallInfo)
# See if we can strip the construction of this
wrapped_f = stmt.args[1]
if isa(wrapped_f, SSAValue)
wrapping_stmt = ir[wrapped_f][:stmt]
if isexpr(wrapping_stmt, :new)
ft = argextype(wrapping_stmt.args[2], ir)
gc_preserve = GCPreserveRewrite(wrapping_stmt.args[2], wrapping_stmt.args[3])
new_argtypes = copy(sig.argtypes)
new_argtypes[1] = ft
sig = Signature(singleton_type(ft), ft, new_argtypes)
end
end
end

# Handle _apply_iterate
sig = inline_apply!(todo, ir, idx, stmt, sig, state)
sig === nothing && return nothing
Expand Down Expand Up @@ -1303,7 +1378,7 @@ function process_simple!(todo::Vector{Pair{Int,Any}}, ir::IRCode, idx::Int, flag
return nothing
end

return stmt, sig
return stmt, sig, gc_preserve
end

function handle_any_const_result!(cases::Vector{InliningCase},
Expand Down Expand Up @@ -1409,13 +1484,13 @@ end

function handle_call!(todo::Vector{Pair{Int,Any}},
ir::IRCode, idx::Int, stmt::Expr, @nospecialize(info::CallInfo), flag::UInt32, sig::Signature,
state::InliningState)
state::InliningState, gc_preserve::Union{GCPreserveRewrite, Nothing})
cases = compute_inlining_cases(info, flag, sig, state)
cases === nothing && return nothing
cases, handled_all_cases, fully_covered, joint_effects = cases
atype = argtypes_to_type(sig.argtypes)
atype === Union{} && return nothing # accidentally actually unreachable
handle_cases!(todo, ir, idx, stmt, atype, cases, handled_all_cases, fully_covered, joint_effects)
handle_cases!(todo, ir, idx, stmt, atype, cases, handled_all_cases, fully_covered, joint_effects, gc_preserve)
end

function handle_match!(cases::Vector{InliningCase},
Expand Down Expand Up @@ -1499,18 +1574,18 @@ end

function handle_cases!(todo::Vector{Pair{Int,Any}}, ir::IRCode, idx::Int, stmt::Expr,
@nospecialize(atype), cases::Vector{InliningCase}, handled_all_cases::Bool, fully_covered::Bool,
joint_effects::Effects)
joint_effects::Effects, gc_preserve::Union{GCPreserveRewrite, Nothing})
# If we only have one case and that case is fully covered, we may either
# be able to do the inlining now (for constant cases), or push it directly
# onto the todo list
if fully_covered && handled_all_cases && length(cases) == 1
handle_single_case!(todo, ir, idx, stmt, cases[1].item)
handle_single_case!(todo, ir, idx, stmt, cases[1].item, gc_preserve)
elseif length(cases) > 0 || handled_all_cases
isa(atype, DataType) || return nothing
for case in cases
isa(case.sig, DataType) || return nothing
end
push!(todo, idx=>UnionSplit(handled_all_cases, fully_covered, atype, cases))
push!(todo, idx=>UnionSplit(handled_all_cases, fully_covered, atype, cases, gc_preserve))
else
add_flag!(ir[SSAValue(idx)], flags_for_effects(joint_effects))
end
Expand Down Expand Up @@ -1629,13 +1704,17 @@ function assemble_inline_todo!(ir::IRCode, state::InliningState)

simpleres = process_simple!(todo, ir, idx, flag, state)
simpleres === nothing && continue
stmt, sig = simpleres
stmt, sig, gc_preserve = simpleres

info = ir.stmts[idx][:info]
if gc_preserve !== nothing && isa(info, GCPreserveDuringCallInfo)
info = info.info
end

# `NativeInterpreter` won't need this, but provide a support for `:invoke` exprs here
# for external `AbstractInterpreter`s that may run the inlining pass multiple times
if isexpr(stmt, :invoke)
gc_preserve === nothing || continue
handle_invoke_expr!(todo, ir, idx, stmt, info, flag, sig, state)
continue
end
Expand All @@ -1652,16 +1731,20 @@ function assemble_inline_todo!(ir::IRCode, state::InliningState)

# handle special cased builtins
if isa(info, OpaqueClosureCallInfo)
gc_preserve === nothing || continue
handle_opaque_closure_call!(todo, ir, idx, stmt, info, flag, sig, state)
elseif isa(info, ModifyOpInfo)
gc_preserve === nothing || continue
handle_modifyop!_call!(ir, idx, stmt, info, state)
elseif sig.f === Core.invoke
gc_preserve === nothing || continue
handle_invoke_call!(todo, ir, idx, stmt, info, flag, sig, state)
elseif isa(info, FinalizerInfo)
gc_preserve === nothing || continue
handle_finalizer_call!(ir, idx, stmt, info, state)
else
# cascade to the generic (and extendable) handler
handle_call!(todo, ir, idx, stmt, info, flag, sig, state)
handle_call!(todo, ir, idx, stmt, info, flag, sig, state, gc_preserve)
end
end

Expand Down
11 changes: 11 additions & 0 deletions Compiler/src/stmtinfo.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,4 +502,15 @@ function add_edges_impl(edges::Vector{Any}, info::GlobalAccessInfo)
push!(edges, info.b)
end

"""
info::GCPreserveCallInfo <: CallInfo

Wraps another CallInfo, indicating that this call came from a looked-through GCPreserveDuring.
"""
struct GCPreserveDuringCallInfo <: CallInfo
info::CallInfo
end
add_edges_impl(edges::Vector{Any}, info::GCPreserveDuringCallInfo) =
add_edges_impl(edges, info.info)

@specialize
5 changes: 5 additions & 0 deletions base/Base.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,11 @@ function start_profile_listener()
ccall(:jl_set_peek_cond, Cvoid, (Ptr{Cvoid},), cond.handle)
end

function print_padded_time(io, mod, maxlen, t)
print(io, rpad(string(mod) * " ", maxlen + 3, "─"))
Base.time_print(io, t * 10^9); println(io)
end

function __init__()
# Base library init
global _atexit_hooks_finished = false
Expand Down
3 changes: 2 additions & 1 deletion base/Base_compiler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ convert(::Type{Any}, Core.@nospecialize x) = x
convert(::Type{T}, x::T) where {T} = x
include("coreio.jl")

import Core: @doc, @__doc__, WrappedException, @int128_str, @uint128_str, @big_str, @cmd
import Core: @doc, @__doc__, WrappedException, @int128_str, @uint128_str, @big_str, @cmd,
GCPreserveDuring

# Export list
include("exports.jl")
Expand Down
19 changes: 19 additions & 0 deletions base/boot.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1141,4 +1141,23 @@ typename(union::UnionAll) = typename(union.body)

include(Core, "optimized_generics.jl")

struct GCPreserveDuring
f::Any
# N.B: This field is opaque - the compiler is allowed to arbitrarily change it
# as long as it has the same GC rooting behavior.
root::Any
GCPreserveDuring(@nospecialize(f), @nospecialize(root)) = new(f, root)
end

# This has special support in inference and codegen and is only ever actually called
# in fallback cases.
function (this::GCPreserveDuring)(args...)
@noinline
r = this.f(args...)
# N.B.: This is correct, but stronger than required. If the call to `f` is deleted,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this is not correct, since it only preserves the root if f returns, and otherwise is UB if f throws

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the compiler isn't allowed to know what f is, so it can't optimize. But yes, fair enough, I guess I could put a try/finally around this

# this may be deleted as well.
donotdelete(this.root)
return r
end

ccall(:jl_set_istopmod, Cvoid, (Any, Bool), Core, true)
11 changes: 8 additions & 3 deletions base/gcutils.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,8 @@ end
"""
GC.@preserve x1 x2 ... xn expr

Mark the objects `x1, x2, ...` as being *in use* during the evaluation of the
expression `expr`. This is only required in unsafe code where `expr`
Mark the objects `x1, x2, ...` as being *in use* during the evaluation of all
calls lexically in the expression `expr`. This is only required in unsafe code where `expr`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still an ambiguity here whether it is forced to be in-use before the first call, particularly if those later get optimized out. I think the previous formulation added an implicit use at the gc-begin, but I don't know if that was implied to be reliable before (or used)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My expectation was that it did not. I.e. @GC.preserve x nothing is allowed to be removed.

*implicitly uses* memory or other resources owned by one of the `x`s.

*Implicit use* of `x` covers any indirect use of resources logically owned by
Expand Down Expand Up @@ -236,7 +236,12 @@ macro preserve(args...)
for x in syms
isa(x, Symbol) || error("Preserved variable must be a symbol")
end
esc(Expr(:gc_preserve, args[end], syms...))
sym = length(syms) == 1 ? only(syms) : Expr(:tuple, syms...)
g = gensym()
esc(quote
$g = $sym
$(Expr(:gc_preserve, args[end], g))
end)
end

"""
Expand Down
Loading