-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: Refactor MethodInstance to allow for more general specialization #54373
base: master
Are you sure you want to change the base?
Conversation
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.
Recently, we added the owner field to CodeInstance, which allowed us
to put all of these into the cache, but that didn't given them support
to be compiled/executed.
Making them compilable/executable is #52964 since that requires tracking the current compiler instance through dynamic dispatch.
I will need to see how this actually interacts with foreign abstract interpreters. While working on #52964 I often needed to know the origin of a CodeInstance. I suppose in your proposal we would add a GPUSpec
or ForeignSpec
? But right-now in many places the code assumes that it DefaultSpec
.
Part of my goal with owner
is that we could re-use the code in many places and reduce the reliance on solutions like spinning up a separate OrcJIT.
One ask that Jeff had for #52964 if we could provide a "separate MethodTable" and outside of generic dispatch that currently works.
if (codeinst->owner != jl_nothing) { | ||
// TODO(vchuravy) native code caching for foreign interpreters | ||
} | ||
else if (jl_atomic_load_relaxed(&codeinst->invoke) != jl_fptr_const_return) { |
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 was needed so that CUDA or other foreign CodeInstances don't bled into the native cache. I am unsure how this alternative handles this.
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.
The idea is to only handle the DefaultSpec here. Additional work may be required in various places to disambiguate.
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.
Yeah you will have to filter out everything that isn't "DefaultSpec"
It requires tracking them trough dynamic dispatch if you want dynamic dispatch to do something other than the default. That's an orthogonal feature to this. This just lets you have multiple specializations with different ABIs for one particular method. They may
Yes
Yes, I need to go through and disambiguate the assumptions, but that's a fair bit of work, so I wanted to get agreement on the direction first.
That's still in scope, but again, not addressed by this. I think the cleanest way to address that is to also make |
Yeah the challenge here is to figure out what fields have meaning and what is needed. What are the requirements for something going through the native pipeline (but not wanting to poison it). The Almost everyone reuses |
Is this behavior already implemented, or is it something that needs to be implemented? Also, I'm curious about how this change should affect the existing cache overload system based on |
It's partially implemented. Some cachine/invalidating/precompile logic is missing.
InternalCodeCache takes a specialization type, which replaces the |
@@ -73,7 +101,7 @@ function setindex!(wvc::WorldView{InternalCodeCache}, ci::CodeInstance, mi::Meth | |||
end | |||
|
|||
function code_cache(interp::AbstractInterpreter) |
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.
Should this now be code_cache(interp:: NativeInterpreter, ...)
? Otherwise foreign interp will silently leak information into the native cache.
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.
Sure, we can force external absint to explicitly declare their cache
I don't yet see the end design and how it will work. It may be that an example of a not Right now I think that owner concept and this mechanism are orthogonal to each other. In particular I am worried that the polymorphism here here will be limited since the C code only expects Maybe the answer is for Base to provide My original attempt when working on the integrated cache started with adding the owner field on the MethodInstance, but that was awkward since we use |
The C code is mostly fine. I've been using it extensively with non-
That would be fine.
Yeah, it's slightly awkward, but you can still do that and just look through the |
* Very WIP: IPO codegen Depends on JuliaLang/julia#54373 and very much a work in progress, but I'm hoping to merge this after a little more work on this to avoid having this yet-again on a long-running unmerged branch. * Fix propagation bug * Start nonlinear codegen * SICM test working * NL SICM * Tests passing (but I don't feel good about it) * New ABI and implicit external test passing * WIP * WIP * Precompile tests * Up Deps * Up deps and adjust tests * Fix rebase version mistake * Adjust to upstream rename
Together with #54899, this PR is intending to replicate the functionality of #54373, which allowed particular specializations to have a different ABI signature than what would be suggested by the MethodInstance's `specTypes` field. This PR handles that by adding a special `ABIOverwrite` type, which, when placed in the `owner` field of a `CodeInstance` instructs the system to use the given signature instead.
Together with #54899, this PR is intending to replicate the functionality of #54373, which allowed particular specializations to have a different ABI signature than what would be suggested by the MethodInstance's `specTypes` field. This PR handles that by adding a special `ABIOverwrite` type, which, when placed in the `owner` field of a `CodeInstance` instructs the system to use the given signature instead.
Overview
This refactors the base MethodInstance data structure to the following:
The
owner
field ofCodeInstance
is removed in favor of usingseparate toplevel
MethodInstance
s.Motivation
This refactor aims to unify a number of recent requirements on the
internal cache. Broadly speaking, we'd like to cache (with proper
invalidation and world age semantics) several classes of data:
traditional MethodInstance/CodeInstance cache)
GPUCompiler use case)
generated function results, some expensive-to-compute intermediate
results in external absint)
return values, more fancy external absint specializations) native and
non native code instances
effect preconditions, see RFC: Effect Preconditions - or - the future of @inbounds #50641)
Now, some of these are expected to be compiled by the standard julia
execution engine (1, 4, 5), some of these have ABIs that match the
type specialization (1, 2, 3), but generally they are not all the same.
Most of these are invalidated along with the original method instance,
but not all. Additionally, some of these (1, 4, 5) have more likely more
edges than the default method-instance leading to over-invalidation.
Recently, we added the
owner
field to CodeInstance, which allowed usto put all of these into the cache, but that didn't given them support
to be compiled/executed. I tried to fix that in #52797, but we didn't
manage to figure out good precompile semantics, so that stalled.
This PR pulls up the
owner
field one level into the type tag ofMethodSpecialization
. This is partly to save the extra pointer inevery CodeInstance, but also to allow partitioning the edges between
native MethodInstances and those used by external abstract interpreters.
There's a few different usage modes:
def
to a Method. In this case, the set ofedges
is completely partitioned between the internal and externalabsint and can be managed according to absint requirements (e.g. this
makes sense if the external absint is using an overlay table)
def
to anotherMethodSpecialization
. Inthis case the set of edges is extended. This is inteded to be used
by absints, which wrap another absint and produce more fine grained
specializations.
Additionally, all non-MethodInstance MethodSpecializations are allowed
in
Expr(:invoke)
. There's some TBD still for how to handle recoveryon reload, but in principle everything should just go through. This thus
closes #52797 as it addresses the same use case, but with proper edge
tracking.
Currently, there's two
D
tags that the runtime system uses.DefaultSpec
, which has the memoization ofsparams
and the variouslock bits that the runtime uses. And
UninferredSpec
which is a singletonand replaces the
owner === :uninferred
CodeInstances introduced in #54362.I anticipate extending this further for effect preconditions and various
finer-than-type specializations in Base.
Current Status
This PR changes the data structures, but does not yet provide the
Core.Compiler
utilities for cache lookup in non-defaultMethodSpecializations. That's on my immediate to do list.
Additionally, the new edge/invalidation logic described above
is not yet implemented. I also haven't tried the #52797 replacement
yet to make sure it actually works properly. I'm putting this up
as a draft to make sure that all relevant package developers have
a chance to complain if I missed something important.