-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add codegen support for invoke(f, ::CodeInstance, args...)
#60442
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?
Add codegen support for invoke(f, ::CodeInstance, args...)
#60442
Conversation
|
Does this/should this do a world age check? |
So just to be clear, I am not proposing introducing a new mechanism here. The new mechanism was already added for v1.12 in #56660. The only thing this PR does is make it so that the mechanism isn't slow to call. I don't really know what one can or cannot do by messing around with the invoke pointer, presumably arbitrary things, but I'm not sure. Regarding a comparison to opaque closures, I think the idea is that these codeinstances are a bit more 'static', rather than combining runtime data with alternative interpretation. Another difference is that a I didn't make the feature though, that was @Keno, so maybe he can motivate it a bit more if the linked PR and my crappy explanation isn't sufficient.
I had initially assumed that |
src/codegen.cpp
Outdated
| } | ||
|
|
||
| // 3. Delegate to emit_invoke for the actual call generation | ||
| *ret = emit_invoke(ctx, argtypes, invoke_args, invoke_args.size(), rt, false); |
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 probably need to emit_invoke to match this error
if (invoke) {
return invoke(args[0], &args[2], nargs - 2, codeinst);
} else {
if (codeinst->owner != jl_nothing || !jl_is_method(codeinst->def->def.value)) {
jl_error("Failed to invoke or compile external codeinst");
}
return jl_gf_invoke_by_method(codeinst->def->def.method, args[0], &args[2], nargs - 1);
Similarly looking at https://github.com/JuliaLang/julia/pull/56660/changes#diff-b6cb5d410b973b5987bc13b1eba0f156fcaece59cfbd6cb12ac503ff44fd13ca
What happens when you provide an "uncompiled" codeinst? You would need a mechanism like in #52964 to go from codeinst->owner to the correct abstract interpreter / compiler instance.
So one avenue forward would be to partially do #52964 but for now error on encountering a dynamic dispatch.
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.
What happens when you provide an "uncompiled" codeinst? You would need a mechanism like in #52964 to go from codeinst->owner to the correct abstract interpreter / compiler instance.
Do we need to worry about that for this PR? I'd really just like to at least get this mechanism working rather than re-design the mechanism (not saying I'm against a redesign or enhancement, but I'd just like to keep this PR focussed on the narrow issue I want to solve: calling overhead if it's possible)
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 think this might be fine to error on (but should be checked)
Your code should replicate whatever the current code does, with potentially deciding some things at compile time
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.
Well you code should match the behavior and emit the same errors as the interpreter/builtin version.
Which has a:
if (!invoke) {
jl_compile_codeinst(codeinst);
|
For the world-age check, how do we do that? It seems that the |
|
Okay, I have
|
(I wrote this with assistance from Gemini since I'm not very used to writing LLVM IR)
This is an attempt to fix #60441. After bumbling around a bit, it seems that the problem is that
invoke(f, ::CodeInstance, args...)calls are not turned intoExpr(:invokestatements in the IR, but remains as:calls to theinvokebuiltin which ends up going through the runtime.There's probably a better way to do this, but the way I found was to just detect these builtin calls in the LLVM IR, and send them to
emit_invoke.It appears to resolve the issue:
Before this PR:
After this PR: