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 Rules, all functioning #382

Merged
merged 23 commits into from
Jul 11, 2023
Merged

Enzyme Rules, all functioning #382

merged 23 commits into from
Jul 11, 2023

Conversation

wsmoses
Copy link
Collaborator

@wsmoses wsmoses commented Apr 13, 2023

Requires https://github.com/EnzymeAD/Enzyme.jl/pull/729/checks

I failed to understand the extension work, so please feel free to rework.

@wsmoses wsmoses requested a review from vchuravy April 13, 2023 21:08
test/testsuite.jl Outdated Show resolved Hide resolved
@vchuravy
Copy link
Member

Also will this only work for the CPU? I would prefer if we could have a generalized implementation that would also work for the GPU backends.

@wsmoses
Copy link
Collaborator Author

wsmoses commented Apr 14, 2023

@vchuravy forward mode is functioning with the rule in CUDA, etc.

reverse mode, is not because of the previously discussed tape issues. I've actually mostly worked around it, however that would need to be in CUDAKernels.jl as a rule registration to be able to get the parent job (And also because mkcontext, etc is apparently not stable from cpu -> gpu etc).

@vchuravy
Copy link
Member

vchuravy commented May 1, 2023

Fails in function verification:

Call parameter type does not match function signature!
{} addrspace(10)* ()* bitcast ({ { {} addrspace(10)*, {} addrspace(10)*, {} addrspace(10)* } ()*, void (i8*)* }* @"_enzyme_reverse_julia_wait_3789'" to {} addrspace(10)* ()*)
 {} addrspace(10)*  %16 = call {} addrspace(10)* @julia_enq_work_3818({} addrspace(10)* ()* bitcast ({ { {} addrspace(10)*, {} addrspace(10)*, {} addrspace(10)* } ()*, void (i8*)* }* @"_enzyme_reverse_julia_wait_3789'" to {} addrspace(10)* ()*)), !dbg !3333

@wsmoses
Copy link
Collaborator Author

wsmoses commented May 1, 2023

This looks lke the rule is no longer triggering?

@vchuravy
Copy link
Member

vchuravy commented May 1, 2023

Ah that's just 1.6 being unsupported

@wsmoses
Copy link
Collaborator Author

wsmoses commented Jul 7, 2023

@vchuravy excludijf nightly which is currently not supported by Enzyme per the swiftcc stuff, this fully builds and runs successfully on CI.

can we merge?

@vchuravy
Copy link
Member

vchuravy commented Jul 7, 2023

@maleadt Reverse CI for CUDA seems to be broken?

Maybe JuliaGPU/CUDA.jl#1970?

@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Patch coverage: 35.55% and project coverage change: -3.72 ⚠️

Comparison is base (9526206) 87.50% compared to head (46a1d1b) 83.78%.

❗ Current head 46a1d1b differs from pull request most recent head f07343f. Consider uploading reports for the commit f07343f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #382      +/-   ##
==========================================
- Coverage   87.50%   83.78%   -3.72%     
==========================================
  Files           8        8              
  Lines         584      623      +39     
==========================================
+ Hits          511      522      +11     
- Misses         73      101      +28     
Impacted Files Coverage Δ
ext/EnzymeExt.jl 30.00% <30.00%> (ø)
src/KernelAbstractions.jl 83.33% <75.00%> (ø)
src/cpu.jl 79.24% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vchuravy
Copy link
Member

vchuravy commented Jul 7, 2023

@wsmoses did you test this with CUDA?

@wsmoses
Copy link
Collaborator Author

wsmoses commented Jul 7, 2023

I did not, but only forward mode has a rule for all backends. Otherwise the PR intentionally just adds reverse mode for CPU. Reason is the tape nested job stuff we discussed, which will take some thinking to solve.

test/enzyme.jl Outdated Show resolved Hide resolved
@vchuravy vchuravy merged commit a53054d into main Jul 11, 2023
14 of 17 checks passed
@vchuravy vchuravy deleted the wmrules branch July 11, 2023 08:08
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.

2 participants