-
Notifications
You must be signed in to change notification settings - Fork 60
Enable users to map from GV to Julia value #746
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #746 +/- ##
==========================================
- Coverage 75.28% 74.49% -0.79%
==========================================
Files 24 24
Lines 3670 3682 +12
==========================================
- Hits 2763 2743 -20
- Misses 907 939 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| @tracepoint "IR generation" begin | ||
| ir, compiled = irgen(job) | ||
| ir, compiled, gv_to_value = irgen(job) |
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 am open to a better name
| if haskey(asm[2], :gv_to_value) | ||
| # TODO: Serializer cannot handle Core.IntrinsicFunction | ||
| # We kinda want Julia to serialize the values in `gv_to_value` in the pkgimg and us just having to store an index | ||
| # for now we just empty them out | ||
| # We would need to remove the initializers from LLVM IR as well to be correct, and then link these in at runtime | ||
| empty!(asm[2].gv_to_value) |
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.
We should likely just give up on caching this for now if !isempty(gv_to_value)
For proper caching, we would want to create an IdDict in the current parent module and instead of saving values we would want to save "offsets/keys" into that IdDict so that Julia performs the pkgimg reolcation for us.
| return list | ||
| end | ||
| end | ||
|
|
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.
😱 For backwards compatibility we need to support the internal arraylist_t format.
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 is pretty cursed... But at least it won't change. Maybe guard the definition with a @static if VERSION too so that it's clear this is only valid for a specific version?
| if ptr in gvalues | ||
| gv_to_value[LLVM.name(gv)] = ptr | ||
| end | ||
| LLVM.initializer!(gv, nothing) |
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.
So we don't necessarily need to delete the initializer here, but it makes the code a bit more consistent.
| return list | ||
| end | ||
| end | ||
|
|
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 is pretty cursed... But at least it won't change. Maybe guard the definition with a @static if VERSION too so that it's clear this is only valid for a specific version?
| # It's valid to call Base.unsafe_pointer_to_objref on values(gv_to_value), | ||
| # but we may not be able to "easily" obtain the pointer back later. | ||
| # (Types, etc, disallow Base.pointer_from_objref on them.) |
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 does this comment apply to?
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 originally had gv_to_value = Dict{String, Any}}() to avoid issues with dangling pointers. Currently, I believe we perma-root objects when we emit the LLVM IR, but I have to make sure that this will work when we deserialize...
So I was storing:
gv_to_value[LLVM.name(gv)] = Base.unsafe_pointer_to_objref(ptr)
but you can't roundtrip all objects (sigh) and for doing the initializer later I need the pointer address again.
Two fold motivation.
Currently, we perform the JIT favourite trick on all globals, we inline the runtime pointers, this makes the resulting code non-relocatable. A concrete example of that would be something like:
The memory location of
:xchanges upon session restart (as do other things like Types, Singletons), and for objects like this Julia performs a simple pointer comparison. So we can actually support this operation on the GPU.Yet, we cannot cache the resulting kernel, or if we do, we will perform the wrong computation. So what we must do instead is to initialize the global after loading it from cache.
Now that is a tricky affair (backend dependent), we can do it on LLVM IR, I believe AMDGPU could also do it, the CPU backends can use something like
JITDylib, but I don't think we can do it on the obj file from Nvidia, and I don't about other backends.So we should likely mark compilations that use globals with runtime pointers as ineligible for caching.
The second motivation stems from Enzyme where we would like to have a mapping from global to Julia object, and it can be hard to figure out which globals are Julia objects and which ones are not.