Adding Radeon support by controlling wave size #1729
Conversation
seanofthemillers
force-pushed
the
feature/seanofthemillers/adding_radeon_support
branch
from
480d181
to
95de1dc
Compare
| @@ -780,8 +780,8 @@ namespace expt | |||
|
|
|||
| // Third: mask off everything but output_segment | |||
| // this is because all output segments are valid at this point | |||
| // (5-segbits), the 5 is since the warp-width is 32 == 1<<5 | |||
| int our_output_segment = get_lane()>>(6-segbits); | |||
| const int log2_warp_size = 32-1-__builtin_clz(warpSize); | |||
There was a problem hiding this comment.
By using warpSize it appear that this is making a runtime value now instead of a compile time value, do we need to worry about the performance implications of this?
There was a problem hiding this comment.
It will add a few instructions and possibly a sgpr load. It probably won't be noticeable, but we can move the log2_warp_size as a constexpr member of the DeviceConstants. Or just wrap these two places in ifdefs. If we keep it dynamic then it should work for Nvidia/other as well - I'm not sure how important that is.
There was a problem hiding this comment.
Given that we're already relying on getting the right warpSize at configure time I think we should keep this compile time if possible. There is a constexpr RAJA::log2 that we can use instead of clz here to keep it constexpr.
Summary
This is a feature. It adds a configuration time control parameter for the default wave size. On AMD MI cards this is generally 64, however on Radeon (gaming) cards this is usually 32. These changes will require the user to know if the card is setup for Wave32 or Wave64.
I also added a fix for the dynamically sized memory allocation which seems to trigger a lot of warnings for ROCm 6.2.
Design review (for API changes or additions---delete if unneeded)
On (date), we reviewed this PR. We discussed the design ideas:
This PR implements 1. and 3. It leaves out 2. for the following reasons