Add a few hooks that make external AbstractInterpreters easier #36317
Conversation
|
I'd been intentionally avoiding this (meaning Defining this to take a |
| # The interpreter that created this inference state. Not looked at by | ||
| # NativeInterpreter. But other interpreters may use this to detect cycles | ||
| interp::AbstractInterpreter | ||
|
|
There was a problem hiding this comment.
This seems like a slight design smell that this other state is getting mixed into into our state here. Why isn't this part of the InferenceParams configuration values?
There was a problem hiding this comment.
Well, it's not really for configuration, it's so that the interpreter can detect that this InferenceState was not created by it and handle it appropriately.
|
I don't want it to take a Method, because I really just want a low-level way to say, "no, invoke this CodeInstance specifically" for debugging and experimenting purposes. I agree it's a bad high-level API, but it's useful for testing. If you think a builtin is too high-level, we could maybe just have it be something you can ccall? |
|
|
||
| // If non-null, rewrite all generic calls to call | ||
| // generic_context(f, args...) instead of f(args...). | ||
| jl_value_t *generic_context; |
There was a problem hiding this comment.
Needs a default value in jl_default_cgparams?
There was a problem hiding this comment.
Yes. Also I realize the comment is outdated, since I settled on nothing rather than null to indicate absence.
…preter This extends hookability to the same-frame comparison in inference's recursion cycle detection. The case I ran into that made this necessary is a recursive, nested AD transform. In this case, inference must detect if two frames have different orders of derivatives (e.g. the primitive for `-`, again calls `-`; the external interpreter makes sure that inference results for these end up in different caches).
This builtin is a low-level mechanism to execute a specific codeinst. Higher-level invokers need to make sure that any world-age limits are observed.
As of #35831, we've had the ability to cache CodeInstances in an external cache and to replace that cache during jl_generate_native (e.g. for GPU compilation). This extends the same capability to CodeInstances to be added to the execution engine.
This adds a new context field that rewrites generic calls like `apply_generic(f, (args...,))` to `apply_generic(context, (f, args...))` during codegen. The intention here is to allow external AbstractInterpreters to provide custom implementations of apply_generic (usually recursing analysis using the same interpreter, but other behavior may be desired). This is a bit of a stopgap solution. I think in the fullness of time, we'll probably want completely custom codegen for generic callsites, to avoid the potential of a double-dispatch impact, but for the moment this allows prototyping.
|
These appear to be unrelated features in each commit—can you make them separate PRs? |
|
That's the point of having them each be a separate commit, so they can be reviewed commit by commit. It didn't seem worth making 4 separate PRs |
|
This whole PR is 65 lines. Are 4 15 line PRs really better, taking 4x the CI time? If the issue is that you like some of the features, but not others? |
|
Yeah, it seems like we generally shouldn't be complicating review for the sake of a CI bot. I don't mind if a PR starts this way (I do that frequently enough myself, especially when one piece of work reveals another), but if you want part of it faster, then it's good to separate that out. Otherwise, it's not the reviewer's fault if the whole thing gets blocked on one piece. And I know I might get yelled at for interleaving thoughts and trying to discuss parts of this while they're entangled with the other parts, so I'd appreciate not needing to try to interleave replies in one PR thread (since GitHub is not particularly great UI for having unrelated discussions in one PR). |
Details, in the individual commit messages. I don't think these are necessarily the correct abstractions longterm, but they are quite useful for prototyping in the short term. With these hooks (and prior work on refactoring the caches), it's possible to have a completely external Julia compiler, complete with its own implementation of apply_generic that will use the correct interpreter for its analysis, while still running on the Julia execution engine.