Skip to content
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

Enzyme: simplify via mixedduplicated #483

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Enzyme: simplify via mixedduplicated #483

merged 1 commit into from
Jun 21, 2024

Conversation

wsmoses
Copy link
Collaborator

@wsmoses wsmoses commented Jun 21, 2024

No description provided.

@@ -150,7 +126,7 @@ module EnzymeExt
args2 = ntuple(Val(N)) do i
Base.@_inline_meta
if args[i] isa Active
Duplicated(Ref(args[i].val), arg_refs[i])
EnzymeCore.MixedDuplicated(args[i].val, arg_refs[i])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct for the CPU, but I think we won't be able to pass MixedDuplicated to the GPU.
Or at least we need an Adapt rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not have those already like we do for the other annotations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently not, now we do here: EnzymeAD/Enzyme.jl#1551

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should double check that this adapts to what we want on CUDA. I don't know if CuRef is right here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure but this is code wouldn’t affect any currently landed cuda support (this is only reverse mode).

So we should likely explore that in michel’s PR?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what about non CUDA GPUs?

@wsmoses
Copy link
Collaborator Author

wsmoses commented Jun 21, 2024

@vchuravy in interim could we get this merged/tagged (is blocking CliMA/Oceananigans.jl#3618 / https://buildkite.com/clima/oceananigans/builds/16126#01903c25-aa5f-4e96-9088-70984b90ebe4 seemingly)

@vchuravy vchuravy merged commit 0e651e4 into main Jun 21, 2024
34 of 36 checks passed
@vchuravy vchuravy deleted the emd branch June 21, 2024 21:20
@vchuravy
Copy link
Member

@wsmoses just so that you aware, this is what Ref get's converted to for cudacall.

https://github.com/JuliaGPU/CUDA.jl/blob/84bb117194d42c5ec58c09e85c312d491fedee68/src/compiler/execution.jl#L165-L175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants