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.

Sign up for GitHub

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

Jump to bottom

ROCKernels: Update to AMDGPU 0.4 #316

Merged
merged 2 commits into from
Sep 14, 2022
Merged

ROCKernels: Update to AMDGPU 0.4 #316

merged 2 commits into from
Sep 14, 2022

Conversation

jpsamaroo
Copy link
Member

No description provided.

@jpsamaroo jpsamaroo added the AMD label Jul 25, 2022
@vchuravy
Copy link
Member

Needs to update the Project.toml?

@jpsamaroo
Copy link
Member Author

Needs to update the Project.toml?

Somehow it missed the push...

So now we have a ROCDevice in AMDGPU.jl. What should we do with ROCKernels.ROCDevice? Any ideas for a better name?

@jpsamaroo
Copy link
Member Author

I switched to KAROCDevice in the meantime. Let me know if anyone has better name ideas 😄

@jpsamaroo
Copy link
Member Author

What's up with the MPI failures?

@vchuravy
Copy link
Member

vchuravy commented Aug 3, 2022

KAROCDevice reads absolutly horrid. You also need to update ROCKernels to 0.4 since this is a breaking change. Maybe ROCmDevice? Can we cheat and just re-use ROCDevice from AMDGPU.jl

@jpsamaroo
Copy link
Member Author

Maybe ROCmDevice? Can we cheat and just re-use ROCDevice from AMDGPU.jl

But it would need to be <: GPU, no?

@vchuravy
Copy link
Member

vchuravy commented Aug 4, 2022

I don't think we use that dispatch rule for much...

@vchuravy
Copy link
Member

vchuravy commented Aug 8, 2022

Can you remove the Manidest.toml you added?

@jpsamaroo
Copy link
Member Author

@vchuravy the <: GPU is necessary to get <: Device dispatches. Would it be reasonable to manually forward these for ROCDevice?

@vchuravy
Copy link
Member

vchuravy commented Aug 8, 2022 via email

@jpsamaroo
Copy link
Member Author

Ok, now all calls generated by the @kernel macro forward down to KernelAbstractions.construct, which is defined for ::Device, and ROCKernels defines its own for ::ROCDevice.

@michel2323 michel2323 mentioned this pull request Aug 17, 2022
Merged
@leios leios mentioned this pull request Sep 7, 2022
Closed
leios
leios approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@leios leios left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It seems to run for me locally. The MPI errors seem to be a general CI problem right now?

Ah, just noticed that we are still using default_device() instead of ROCDevice(). I am using ROCDevice() = AMDGPU.default_device() locally and that seems to work.

@jpsamaroo
Copy link
Member Author

Yeah, the MPI failures seem to be something specific to CI.

Let's hold off on merging this until:

  • We've tested that this works properly for CUDAKernels (since we change some internals to support using AMDGPU.ROCDevice
  • We've added the ROCDevice() ctor to AMDGPU, so that we can keep doing that (it's much nicer for users)

@jpsamaroo
Copy link
Member Author

JuliaGPU/AMDGPU.jl#280

@jpsamaroo jpsamaroo force-pushed the jps/amdgpu-0.4 branch 2 times, most recently from 6e8486f to 15d6b80 Compare September 8, 2022 16:29
@vchuravy vchuravy closed this Sep 12, 2022
@vchuravy vchuravy reopened this Sep 12, 2022
@codecov
Copy link

codecov bot commented Sep 12, 2022 •
edited
Loading

Codecov Report

Merging #316 (2fa4134) into master (ba43cbd) will increase coverage by 0.78%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   71.66%   72.45%   +0.78%     
==========================================
  Files          14       14              
  Lines        1020     1031      +11     
==========================================
+ Hits          731      747      +16     
+ Misses        289      284       -5
Impacted Files Coverage Δ
lib/ROCKernels/src/ROCKernels.jl 0.00% <0.00%> (ø)
src/KernelAbstractions.jl 90.14% <100.00%> (+3.18%) ⬆️
src/macros.jl 94.33% <100.00%> (+1.79%) ⬆️
src/cpu.jl 87.20% <0.00%> (+4.79%) ⬆️
lib/KernelGradients/test/testsuite.jl 25.00% <0.00%> (+25.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vchuravy vchuravy merged commit 67122c1 into master Sep 14, 2022
@vchuravy vchuravy deleted the jps/amdgpu-0.4 branch September 14, 2022 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leios leios leios approved these changes

Assignees
No one assigned
Labels
AMD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jpsamaroo @vchuravy @leios