-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add tape_type query given a parent compiler job context #1104
Conversation
@michel2323 @vchuravy re parent job/GPU things, if you want to try that out. |
Looks about right. |
3ddc4a3
to
fb74dc4
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1104 +/- ##
==========================================
- Coverage 75.13% 74.75% -0.39%
==========================================
Files 35 35
Lines 10397 10442 +45
==========================================
- Hits 7812 7806 -6
- Misses 2585 2636 +51 ☔ View full report in Codecov by Sentry. |
c5db69c
to
57e884e
Compare
@wsmoses Trying to get GPUs in KA going with https://github.com/JuliaGPU/KernelAbstractions.jl/tree/enz_rev_gpu . I use the 'reverse_gpu.jl` script for development. Now, the compiler crashes. In this PR I did this notable (uninformed) changes:
I believe the GPU rules don't actually belong in KA but in the backends like CUDA.jl, but let's ignore that for the moment. |
The log. |
Yeah, you could probably use |
57e884e
to
8bf56ea
Compare
d4fdc35
to
8beef7a
Compare
a0196f3
to
8083509
Compare
|
8701270
to
1fc9b9b
Compare
src/Enzyme.jl
Outdated
|
||
# output | ||
|
||
(7.26, 2.2, [3.3]) | ||
``` | ||
""" | ||
@inline function autodiff_deferred_thunk(::ReverseModeSplit{ReturnPrimal,ReturnShadow,Width,ModifiedBetweenT, RABI}, ::Type{FA}, ::Type{A}, args...) where {FA<:Annotation, A<:Annotation, ReturnPrimal,ReturnShadow,Width,ModifiedBetweenT, RABI<:ABI} | ||
@inline function autodiff_deferred_thunk( |
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.
Should we just delete this function? We could provide a helper that calls tape_type
internally, but that seems less helpful.
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.
Yeah I'm fine nuking this
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.
Yeah I'm fine nuking this
Nuked and changed the API. Moving TapeType to second argument.
src/Enzyme.jl
Outdated
AugT = Compiler.AugmentedForwardThunk{Ptr{Cvoid}, FA, A2, TT, Val{width}, Val(ReturnPrimal), TapeType} | ||
@assert AugT == typeof(nondef[1]) | ||
AdjT = Compiler.AdjointThunk{Ptr{Cvoid}, FA, A2, TT, Val{width}, TapeType} | ||
@assert AdjT == typeof(nondef[2]) | ||
AugT(primal_ptr), AdjT(adjoint_ptr) | ||
end | ||
|
||
@inline function autodiff_deferred_thunk( |
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.
Maybe move TapeType after FA
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.
autodiff_deferred_thunk
is variadic. As long as we keep both methods I think this could lead to some trouble to distinguish between the two in a call. Or maybe not?
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 is A in contrast to A2
ada6ed4
to
854b6c5
Compare
Rebased and cleaned up. There are no tests yet for |
Yeah tests would be nice
…On Thu, Feb 22, 2024 at 11:45 AM Michel Schanen ***@***.***> wrote:
Rebased and cleaned up. There are no tests yet for autodiff_deferred_thunk.
Should I work on this before merging?
—
Reply to this email directly, view it on GitHub
<#1104 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJTUXBHRGDNOMWVEUFO5ZLYU6N3ZAVCNFSM6AAAAAA6HVPW46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGE2DAMBRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Added a test. That test with CuArray fails, though, but I guess that's expected as it's not a kernel but uses broadcast. |
@wsmoses What still needs to be done 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.
This looks good to me, except the Julia1.6 errors
@vchuravy any thoughts (maybe from the jit changes?)
@michel2323 bumping this, once 1.6 is fixed this should be good |
d2c78fd
to
4711e88
Compare
87079ee
to
d47478b
Compare
@wsmoses Was indeed an easy fix. I merged back |
I don't know about the this error after the merge. Can you look into this?
Edit: Seems to be an issue on |
LGTM. I can't actually approve this since I'm the one who made the PR. @vchuravy can you give it a once over (in particular the orcv1), approve, and let's merge! |
and yeah @michel2323 that's a "we should jll bump soon, but I haven't gotten to, since at visit weekend, and there's some other complex number stuff that should also land" |
He wrote the orcv1 stuff. That one change I did was for sure only a typo level mistake. Edit: Actually, he wrote 95% of the PR. |
Okay I'll just force merge it then. |
Still needs a test, but pinging @vchuravy for review/thoughts