Allow Cassette (et al.) to participate in backedge system #32237
Conversation
| Core.println(edges === nothing) | ||
| Core.println(edges) | ||
| Core.println(typeof(edges)) | ||
| edges = edges::Vector{MethodInstance} |
There was a problem hiding this comment.
should be a Vector{Any}, since edges only sometimes end at a MethodInstance
There was a problem hiding this comment.
In the Cassette use case, don't they always end at a MethodInstance (because Cassette by nature gets concrete values from the generated function)?
base/compiler/typeinfer.jl
Outdated
| @assert isa(edge, MethodInstance) | ||
| ccall(:jl_method_instance_add_backedge, Cvoid, (Any, Any), edge, caller) | ||
| end | ||
| empty!(edges) |
There was a problem hiding this comment.
Do we ever look at this twice? I figure after this the CodeInfo holds the inferred result and we don't ever come back here again (and nobody caches the CodeInfo after it comes out of the generated function).
There was a problem hiding this comment.
mutating it here though just means we had to make a copy of it earlier though, so it's strictly worse. the memory won't be reclaimed until gc regardless
There was a problem hiding this comment.
fair. Perhaps what I meant here was frame.src.edges = nothing?
Keno
force-pushed
the
kf/cassetteedges
branch
2 times, most recently
from
eafcd93 to
851d9c0
Compare
|
@vtjnash says this is the right way to do it, so removing the WIP |
Keno
changed the title
src/jltypes.c
Outdated
| jl_bool_type), | ||
| 0, 1, 17); | ||
| jl_perm_symsvec(18, | ||
| "code", |
There was a problem hiding this comment.
code editor seems to be auto-reindenting this block unnecessarily? The intent of the current alignment is to prevent these sorts of excess diff noise, so that adding removing fields and or changing variable names doesn't require excess conflict regions and an indent-aware find-replace editor.
src/jltypes.c
Outdated
| jl_any_type, | ||
| jl_any_type, | ||
| jl_ulong_type, | ||
| jl_ulong_type, |
There was a problem hiding this comment.
👍 I don't think we used to have the ulong type available here previously. Can we change all of the other similar slots too now?
|
Also, looks like this could be a breaking change, per #32028 |
|
Separated out the long->ulong change into #32241. |
Adds an extra field in `CodeInfo` that allows users (Cassette, et al.) to specify dependencies (as forward edges to `MethodInstance`s) that should be turned into backedges once the CodeInfo is passed over by inference. The test includes a minimal implementation of the Cassette mechansim to exercise this code path.
|
Rebased and squashed. @vtjnash good to go? |
|
dance party in the JuliaLab, thanks Keno and Jameson! |
I'm looking into solving the long-standing backedge issue for Cassette
(where the overdubbed functions don't get invalidated if the function
they are transforming gets invalidated). I figured the best place to start
would a test that shows the issue and a minimal attempt at a fix. I may
very well be using the wrong object to attach the backedge here, but it's
a working starting point.
@vtjnash still needs to tell me what the correct way to do this is.