-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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.
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
base: master
Are you sure you want to change the base?
Conversation
# Disclaimer This is an attempt to resolve #59124. It is incompletely implemented, but I think the key pieces are implemented in each of the involved subsystems. The idea of this PR is to facilitate discussion of this solution while making sure there aren't any major unknown unknowns (which I don't think there are). # Design This PR adds a new struct defined as: ``` 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, # this may be deleted as well. donotdelete(this.root) return r end ``` The idea is that the call method here exists for semantics, but is essentially never actually used. Instead, inference treats it transparently (as if a call to the wrapped function) and codegen codegens it as the original call plus a `jl_roots` operand bundle (which our LLVM passes already supported, because it's used in the gc preserve implementation for foreigncall). # Key notes for relevant subsystems ## Lowering In lowering, the `gc_preserve` syntax form is expanded by first expanding its first argument and then rewriting every call in the expansion from `(call f ,@Args)` to `(call (new (top GCPreserveDuring) f preservee) ,@Args)`. As lowering is lexical, this will apply to every call in the lexical scope of the macro (and no other calls). Now, of course the preservee itself is not lexical. Lowering itself does not care and simply copies whatever value you put there. However, to preserve the existing semantics of the `@GC.preserve` macro, the implementation of the macro has changed to introduce and intermediate slot (thus capturing the value of the preserved slot at entry to the macro). As a result, the case where the entry to the macro is not evaluated for any reason now errors with an UndefVarError, rather than crashing in the optimizer. ## Inlining Inlining for calls of `GCPreserveDuring` is adjusted to apply the same `GCPreserveDuring` to every statement being inlined. ## :invoke_modify The `:invoke_modify` expr head gains a new case for invokes of `GCPreserveDuring`, which if recognized as such are treated the same as regular `:invoke`s, except that the preservation is applied. I should note though that this causes problems for existing `:invoke_modify`', since there is a semantic ambiguity of whether they are permitted to be decayed to `:invoke`. Currently this is handled by just bailing in this case, but we may want a new expr head instead. ## Codegen Codegen is the least implemented. In the current design, the `jl_cgval_t` struct gains a new `wrapped_typ` field that if set indicates that this is a GCPreserveDuring of the indicated type. `boxed` would reconstitute it as such and the various `emit_` wrappers would turn it back into a non-wrapped `jl_cgval_t` and treat the preservees as appropriate. I'm not sure this'll be the final design. In any case, the preservees are turned into jl_roots operand bundles.
It occurs to me that this may be better handled by wrapping the invoked method instance in a marker, (could even be |
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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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` |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
@@ -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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Note that this lowering may change in JuliaLang/julia#59129
Note that this lowering may change in JuliaLang/julia#59129
Note that this lowering may change in JuliaLang/julia#59129
Note that this lowering may change in JuliaLang/julia#59129
* fix showing MacroExpansionError when sourceref isa LineNumberNode * Support K"gc_preserve" (Note that this lowering may change in JuliaLang/julia#59129) * Support `Expr(:isglobal)` * Support K"cmdstring" (just convert it to a macrocall) * Delete two-arg dot form check (The number of forms isn't bounded) * Don't fail on `Expr(:inbounds)` * List known meta forms + improve compat.jl errors * Turn off flisp fallback for now --------- Co-authored-by: Claire Foster <[email protected]>
Disclaimer
This is an attempt to resolve #59124. It is incompletely implemented, but I think the key pieces are implemented in each of the involved subsystems. The idea of this PR is to facilitate discussion of this solution while making sure there aren't any major unknown unknowns (which I don't think there are).
Design
This PR adds a new struct defined as:
The idea is that the call method here exists for semantics, but is essentially never actually used. Instead, inference treats it transparently (as if a call to the wrapped function) and codegen codegens it as the original call plus a
jl_roots
operand bundle (which our LLVM passes already supported, because it's used in the gc preserve implementation for foreigncall).Key notes for relevant subsystems
Lowering
In lowering, the
gc_preserve
syntax form is expanded by first expanding its first argument and then rewriting every call in the expansion from(call f ,@args)
to(call (new (top GCPreserveDuring) f preservee) ,@args)
. As lowering is lexical, this will apply to every call in the lexical scope of the macro (and no other calls).Now, of course the preservee itself is not lexical. Lowering itself does not care and simply copies whatever value you put there. However, to preserve the existing semantics of the
@GC.preserve
macro, the implementation of the macro has changed to introduce and intermediate slot (thus capturing the value of the preserved slot at entry to the macro).As a result, the case where the entry to the macro is not evaluated for any reason now errors with an UndefVarError, rather than crashing in the optimizer.
Inlining
Inlining for calls of
GCPreserveDuring
is adjusted to apply the sameGCPreserveDuring
to every statement being inlined.:invoke_modify
The
:invoke_modify
expr head gains a new case for invokes ofGCPreserveDuring
, which if recognized as such are treated the same as regular:invoke
s, except that the preservation is applied. I should note though that this causes problems for existing:invoke_modify
', since there is a semantic ambiguity of whether they are permitted to be decayed to:invoke
. Currently this is handled by just bailing in this case, but we may want a new expr head instead.Codegen
Codegen is the least implemented. In the current design, the
jl_cgval_t
struct gains a newwrapped_typ
field that if set indicates that this is a GCPreserveDuring of the indicated type.boxed
would reconstitute it as such and the variousemit_
wrappers would turn it back into a non-wrappedjl_cgval_t
and treat the preservees as appropriate. I'm not sure this'll be the final design.In any case, the preservees are turned into jl_roots operand bundles.