-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Ensure CI from external MI and foreign abs interpreter survive precompilation #60747
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?
Conversation
Co-authored-by: Valentin Churavy <[email protected]> (cherry picked from commit b33ce72)
17711b9 to
9a8e5d2
Compare
|
I expect that we also need: diff --git a/src/staticdata_utils.c b/src/staticdata_utils.c
index 16ebcd4b4ee..ad6aceb411e 100644
--- a/src/staticdata_utils.c
+++ b/src/staticdata_utils.c
@@ -502,7 +502,7 @@ static jl_array_t *queue_external_cis(jl_array_t *list, jl_query_cache *query_ca
int dispatch_status = jl_atomic_load_relaxed(&m->dispatch_status);
if (!(dispatch_status & METHOD_SIG_LATEST_WHICH))
continue; // ignore replaced methods
- if (ci->owner == jl_nothing && jl_atomic_load_relaxed(&ci->inferred) && jl_is_method(m) && jl_object_in_image((jl_value_t*)m->module)) {
+ if (jl_atomic_load_relaxed(&ci->inferred) && jl_is_method(m) && jl_object_in_image((jl_value_t*)m->module)) {
int found = has_backedge_to_worklist(mi, &visited, &stack, query_cache);
assert(found == 0 || found == 1 || found == 2);
assert(stack.len == 0);but I want to ensure that the new test fails. |
|
|
||
| include("precompile_utils.jl") | ||
|
|
||
| precompile_test_harness() do load_path |
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.
@aviatesk I added a new test here instead of adding to precopile_absintX since the structure is slightly different.
The test comes from
https://github.com/JuliaGPU/GPUCompiler.jl/blob/master/test/native/precompile.jl
where we have a "compiler" package and a "user" package, and we are adding a code instance to a method instance that is already owned somewhere else identity(::Any) which is why we need the PrecompileTools-esque setup.
test/precompile_utils.jl
Outdated
| function check_presence(mi, token) | ||
| ci = isdefined(mi, :cache) ? mi.cache : nothing | ||
| while ci !== nothing | ||
| if ci.owner === token && ci.max_world == typemax(UInt) |
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.
Something probably went horribly wrong if ci.max_world != WORLD_AGE_REVALIDATION_SENTINEL, since that means the Compiler made a mistake and forgot to get it validated
Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
| ci = isdefined(mi, :cache) ? mi.cache : nothing | ||
| while ci !== nothing | ||
| if ci.owner === token | ||
| @test ci.max_world == Base.ReinferUtils.WORLD_AGE_REVALIDATION_SENTINEL || ci.min_world <= 1 |
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.
@vtjnash I am a bit confused about this query:
- There could be more than one entry? We should select the one that is valid in the current world.
The values I am observing here are:
ci = CodeInstance for MethodInstance for ExampleUser.square(::Float64)
ci.max_world = 0xffffffffffffffff
ci.min_world = 0x0000000000009b8c
ci = CodeInstance for MethodInstance for identity(::Any)
ci.max_world = 0xffffffffffffffff
ci.min_world = 0x0000000000009b8c
Which does look to me like they are correctly integrated? But they of course fail the test, since max_world != WORLD_AGE_REVALIDATION_SENTINAL and min_world is not <=1
|
@MasonProtter given #60442 could you write some tests that check that foreign code instances that are part of a |
Forward-port of #60741