Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Taking world ages seriously #394
Taking world ages seriously #394
Changes from all commits
1bfd0ab
d2568aa
b6f0d8d
b595860
0146779
366ab09
4ced3da
ad6d24c
7223e6b
5cafd78
e022770
84e794e
648b38f
dc3bf57
7a74333
5d2604f
d0d1c37
18cc0d4
1ac5160
b91b6a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 seems to be trying to always return the value of the world for the
get_world
function definition, not the lookup world?But this also seems very risky, as you are claiming to the compiler that this
world
value is a constant, but we know for a fact (based on the edges, min_world, and max_world) that the meaning ofworld
numbers is certain to be dynamic.The appropriate token to return here might be the MethodInstance object
mi
? That would encapsulate fully the MethodMatch lookup result, and exactly represent the result of the lookup for a long as that is the correct compilation target?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.
Would it be fine to return the generator's world (post-JuliaLang/julia#48766)? That's what I'm doing in #403.
Returning the
mi
could work I guess, but would need some refactoring. We're currently returning the world so that we can use it to construct a FunctionSpec, which is the object that we use to look-up the code to compile:GPUCompiler.jl/src/interface.jl
Lines 62 to 71 in 7e0a6ff
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.
Note that the reason we use a separate struct is that it used to carry more fields. Now that they're gone, maybe we should just use MethodInstance instead of FunctionSpec, as @vchuravy notes.
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.
No, it is never allowable to expose the user to worlds from the compiler and vice versa. It violates the contract that only edges are observable effects.
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.
OK. We do however need the world age for some cases though, e.g., to look-up in our CodeInstance cache, and to pass to
jl_create_native
(not sure what to use here). I would assume that both need to be the world age that we need to generate code for (i.e. what we're returning now from this generator), and not the Method's primary world. Or what do you suggest to use 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.
AFAIU using
mi.def.primary_world
won't work because that returns too early of a world:i.e. using
map
'sprimary_world
and passing that to, sayjl_create_native
won't work because we don't havefoo
there. That's why I was 'leaking' the world from the generated function before.Any thoughts?
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.
Shouldn't that just be the tls_world_age ccall?
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.
These seems overly strict, as it is supposed to be perfectly reasonable to run a Method in any world, irrespective of its primary_world and delete_world values (which only refer to the lookup of said Method).
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 would that look like (in the context of GPUCompiler)? I put that there because
jl_create_native
uses the same check to decide whether to emit any code: https://github.com/JuliaLang/julia/blob/7341fb9517d290be02fdc54ae453999843a0dc7e/src/aotcompile.cpp#L332-L335There 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.
oh, that is just a heuristic decision, since our caller isn't designed to be particularly reliable about filtering those earlier (since we do want it to compile for both the current and typeinf worlds when applicable)