-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Extend invoke
to accept CodeInstance
#56660
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# This file is machine-generated - editing it directly is not advised | ||
|
||
julia_version = "1.12.0-DEV" | ||
manifest_format = "2.0" | ||
project_hash = "84f495a1bf065c95f732a48af36dd0cd2cefb9d5" | ||
|
||
[[deps.Compiler]] | ||
path = "../.." | ||
uuid = "807dbc54-b67e-4c79-8afb-eafe4df6f2e1" | ||
version = "0.0.2" | ||
|
||
[[deps.CompilerDevTools]] | ||
path = "." | ||
uuid = "92b2d91f-d2bd-4c05-9214-4609ac33433f" | ||
version = "0.0.0" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
name = "CompilerDevTools" | ||
uuid = "92b2d91f-d2bd-4c05-9214-4609ac33433f" | ||
|
||
[deps] | ||
Compiler = "807dbc54-b67e-4c79-8afb-eafe4df6f2e1" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
module CompilerDevTools | ||
|
||
using Compiler | ||
using Core.IR | ||
|
||
struct SplitCacheOwner; end | ||
struct SplitCacheInterp <: Compiler.AbstractInterpreter | ||
world::UInt | ||
inf_params::Compiler.InferenceParams | ||
opt_params::Compiler.OptimizationParams | ||
inf_cache::Vector{Compiler.InferenceResult} | ||
function SplitCacheInterp(; | ||
world::UInt = Base.get_world_counter(), | ||
inf_params::Compiler.InferenceParams = Compiler.InferenceParams(), | ||
opt_params::Compiler.OptimizationParams = Compiler.OptimizationParams(), | ||
inf_cache::Vector{Compiler.InferenceResult} = Compiler.InferenceResult[]) | ||
new(world, inf_params, opt_params, inf_cache) | ||
end | ||
end | ||
|
||
Compiler.InferenceParams(interp::SplitCacheInterp) = interp.inf_params | ||
Compiler.OptimizationParams(interp::SplitCacheInterp) = interp.opt_params | ||
Compiler.get_inference_world(interp::SplitCacheInterp) = interp.world | ||
Compiler.get_inference_cache(interp::SplitCacheInterp) = interp.inf_cache | ||
Compiler.cache_owner(::SplitCacheInterp) = SplitCacheOwner() | ||
|
||
import Core.OptimizedGenerics.CompilerPlugins: typeinf, typeinf_edge | ||
@eval @noinline typeinf(::SplitCacheOwner, mi::MethodInstance, source_mode::UInt8) = | ||
Base.invoke_in_world(which(typeinf, Tuple{SplitCacheOwner, MethodInstance, UInt8}).primary_world, Compiler.typeinf_ext, SplitCacheInterp(; world=Base.tls_world_age()), mi, source_mode) | ||
|
||
@eval @noinline function typeinf_edge(::SplitCacheOwner, mi::MethodInstance, parent_frame::Compiler.InferenceState, world::UInt, source_mode::UInt8) | ||
# TODO: This isn't quite right, we're just sketching things for now | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment could use some expanded explanation. Why are we merging something that is wrong: is it merely something incomplete or are we saying this would be unsound to use? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed the pieces of this PR that do anything with the optimized generics, but I've kept the definitions, because they need to be in Core. That way an upgradable compiler stdlib can start making them do something. |
||
interp = SplitCacheInterp(; world) | ||
Compiler.typeinf_edge(interp, mi.def, mi.specTypes, Core.svec(), parent_frame, false, false) | ||
end | ||
|
||
# TODO: This needs special compiler support to properly case split for multiple | ||
# method matches, etc. | ||
@noinline function mi_for_tt(tt, world=Base.tls_world_age()) | ||
interp = SplitCacheInterp(; world) | ||
match, _ = Compiler.findsup(tt, Compiler.method_table(interp)) | ||
Base.specialize_method(match) | ||
end | ||
|
||
function with_new_compiler(f, args...) | ||
tt = Base.signature_type(f, typeof(args)) | ||
vtjnash marked this conversation as resolved.
Show resolved
Hide resolved
|
||
world = Base.tls_world_age() | ||
new_compiler_ci = Core.OptimizedGenerics.CompilerPlugins.typeinf( | ||
SplitCacheOwner(), mi_for_tt(tt), Compiler.SOURCE_MODE_ABI | ||
) | ||
invoke(f, new_compiler_ci, args...) | ||
end | ||
|
||
export with_new_compiler | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,15 @@ | |
# especially try to make sure any recursive and leaf functions have concrete signatures, | ||
# since we won't be able to specialize & infer them at runtime | ||
|
||
activate_codegen!() = ccall(:jl_set_typeinf_func, Cvoid, (Any,), typeinf_ext_toplevel) | ||
function activate_codegen!() | ||
ccall(:jl_set_typeinf_func, Cvoid, (Any,), typeinf_ext_toplevel) | ||
Core.eval(Compiler, quote | ||
let typeinf_world_age = Base.tls_world_age() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The value returned from this function is not a legal constant to embed into IR (it is only a runtime value and can change meaning over time and precompile), and so this can lead to incorrect behavior There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This definition is not meant to survive through package precompilation - only sysimg. It's equivalent to the current mechanism for switching to inference world age. That said, if we can come up with a better precompile safe pattern (see above), I'm happy to use it here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Responding to all 3 comments at once here, I think the main question to answer is just what the API demo looks like, which may then directly provide the answer as to what to call it, which world is relevant to it, and so on. I think we just either need to try to completely merge that within the next 2 weeks, or delete the small stub code in Core (just for the release) so that we aren't committing to the API before it does anything useful and can be tested. Hopefully we can do the former though |
||
@eval Core.OptimizedGenerics.CompilerPlugins.typeinf(::Nothing, mi::MethodInstance, source_mode::UInt8) = | ||
Base.invoke_in_world($(Expr(:$, :typeinf_world_age)), typeinf_ext_toplevel, mi, Base.tls_world_age(), source_mode) | ||
end | ||
end) | ||
end | ||
|
||
function bootstrap!() | ||
let time() = ccall(:jl_clock_now, Float64, ()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -268,6 +268,17 @@ end | |
add_edges_impl(edges::Vector{Any}, info::UnionSplitApplyCallInfo) = | ||
for split in info.infos; add_edges!(edges, split); end | ||
|
||
""" | ||
info::InvokeCICallInfo | ||
|
||
Represents a resolved call to `Core.invoke` targeting a `Core.CodeInstance` | ||
""" | ||
struct InvokeCICallInfo <: CallInfo | ||
edge::CodeInstance | ||
end | ||
add_edges_impl(edges::Vector{Any}, info::InvokeCICallInfo) = | ||
add_one_edge!(edges, info.edge) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This adds a dispatch edge, which is wrong for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, it does appear to do what I want, which is to invalidate the call if the target CodeInstance gets its bounds capped, because we're reading that bound information in inference. What edge type are you suggesting? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Theoretically that is a recursion-only edge, which we do not have the ability to represent precisely, but the function to add them is called |
||
|
||
""" | ||
info::InvokeCallInfo | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,4 +54,31 @@ module KeyValue | |
function get end | ||
end | ||
|
||
# Compiler-recognized intrinsics for compiler plugins | ||
""" | ||
module CompilerPlugins | ||
|
||
Implements a pair of functions `typeinf`/`typeinf_edge`. When the optimizer sees | ||
a call to `typeinf`, it has license to instead call `typeinf_edge`, supplying the | ||
current inference stack in `parent_frame` (but otherwise supplying the arguments | ||
to `typeinf`). typeinf_edge will return the `CodeInstance` that `typeinf` would | ||
have returned at runtime. The optimizer may perform a non-IPO replacement of | ||
the call to `typeinf` by the result of `typeinf_edge`. In addition, the IPO-safe | ||
fields of the `CodeInstance` may be propagated in IPO mode. | ||
""" | ||
module CompilerPlugins | ||
""" | ||
typeinf(owner, mi, source_mode)::CodeInstance | ||
|
||
Return a `CodeInstance` for the given `mi` whose valid results include at | ||
the least current tls world and satisfies the requirements of `source_mode`. | ||
""" | ||
function typeinf end | ||
|
||
""" | ||
typeinf_edge(owner, mi, parent_frame, world, abi_mode)::CodeInstance | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like it could use a different name, since it doesn't appear to be compatible with the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to change the name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
""" | ||
function typeinf_edge end | ||
end | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1587,6 +1587,28 @@ JL_CALLABLE(jl_f_invoke) | |
if (!jl_tuple1_isa(args[0], &args[2], nargs - 1, (jl_datatype_t*)m->sig)) | ||
jl_type_error("invoke: argument type error", argtypes, arg_tuple(args[0], &args[2], nargs - 1)); | ||
return jl_gf_invoke_by_method(m, args[0], &args[2], nargs - 1); | ||
} else if (jl_is_code_instance(argtypes)) { | ||
jl_code_instance_t *codeinst = (jl_code_instance_t*)args[1]; | ||
jl_callptr_t invoke = jl_atomic_load_acquire(&codeinst->invoke); | ||
if (jl_tuple1_isa(args[0], &args[2], nargs - 2, (jl_datatype_t*)codeinst->def->specTypes)) { | ||
vtjnash marked this conversation as resolved.
Show resolved
Hide resolved
|
||
jl_type_error("invoke: argument type error", codeinst->def->specTypes, arg_tuple(args[0], &args[2], nargs - 2)); | ||
} | ||
if (jl_atomic_load_relaxed(&codeinst->min_world) > jl_current_task->world_age || | ||
jl_current_task->world_age > jl_atomic_load_relaxed(&codeinst->max_world)) { | ||
jl_error("invoke: CodeInstance not valid for this world"); | ||
} | ||
if (!invoke) { | ||
jl_compile_codeinst(codeinst); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this always compiles code correctly, since normally the runtime expects that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you be more specific. Are you concerned that there are codeinstances on which this isn't legal to call? If so, I think we should flag them separately. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume it normally expects the |
||
invoke = jl_atomic_load_acquire(&codeinst->invoke); | ||
} | ||
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); | ||
vtjnash marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
if (!jl_is_tuple_type(jl_unwrap_unionall(argtypes))) | ||
jl_type_error("invoke", (jl_value_t*)jl_anytuple_type_type, argtypes); | ||
|
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 don't see how this
invoke_in_world
could ever be useful, and only how it could be harmful. Either this function got successfully called, in which case the world is apparently already correct for calling it, or the caller failed to run this in the correct world, in which case the world error should have happened before the call could reach here.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 objective is to switch to a fixed world for invalidation avoidance, while also being compatible with precompilation. Happy to take alternative suggestions for how to accomplish this.