[SYCL][HIP] Use valid cast for CUdeviceptr in HIP PI #6207
Conversation
Replace invalid reinterpret_cast with static_cast for CUdeviceptr in hip_piextMemGetNativeHandle when building HIP PI on CUDA machine; Signed-off-by: Lukas Sommer <[email protected]>
| @@ -2131,8 +2131,7 @@ pi_result hip_piMemGetInfo(pi_mem memObj, pi_mem_info queriedInfo, | |||
| /// \return PI_SUCCESS | |||
| pi_result hip_piextMemGetNativeHandle(pi_mem mem, | |||
| pi_native_handle *nativeHandle) { | |||
| *nativeHandle = | |||
| reinterpret_cast<pi_native_handle>(mem->mem_.buffer_mem_.get()); | |||
| *nativeHandle = static_cast<pi_native_handle>(mem->mem_.buffer_mem_.get()); | |||
There was a problem hiding this comment.
From the issue:
Replacing the reinterpret_cast by static_cast avoids the compilation error, but might be unsafe due to narrowing.
Can we have a check that upper bits are zero and return some failure code if that's not the case?
There was a problem hiding this comment.
Hi @aelovikov-intel, thanks for the review.
I've added a check to make sure that the upper bits are zero.
Signed-off-by: Lukas Sommer <[email protected]>
|
@aelovikov-intel: It seems that the workflow needs approval to run: Can you give this approval to trigger the workflow? |
bader
changed the title
Signed-off-by: Lukas Sommer <[email protected]>
|
@aelovikov-intel: The workflow failed for HIP PI on AMD platform, sorry. I pushed a change that should hopefully fix this, can you approve the workflow again, please? |
| } | ||
| } | ||
| *nativeHandle = static_cast<pi_native_handle>(mem->mem_.buffer_mem_.get()); | ||
| #elif defined(__HIP_PLATFORM_AMD__) |
There was a problem hiding this comment.
Why is the code above not suitable for __HIP_PLATFORM_AMD__ ?
There was a problem hiding this comment.
can you approve the workflow again, please?
My outlook filters for github notifications aren't perfect yet and different notifications end up in different folders. Just saw your comment.
I'd like to understand what exactly is wrong with the AMD platform because it's not obvious to me. The upperbits handling looks like the right thing to me regardless of the problem.
There was a problem hiding this comment.
Just FYI. I approved the workflow both times.
There was a problem hiding this comment.
@bader Thanks for approving the workflows! I'm not sure why the second run failed, I don't see how my changes could affect a reduction test on OpenCL on Intel GPU.
@aelovikov-intel: The definition of hipDeviceptr_t (which is the same as native_type, the return of mem->mem_buffer_mem_.get()) is different on Nvidia and AMD platforms.
On Nvidia/CUDA platforms, it is an alias for CUdeviceptr, which is unsigned long long, so a static_cast is required to convert it to pi_native_handle (unsigned long).
On AMD platforms, it is an alias for void*, and static_cast of void* to unsigned long are invalid, so a reinterpret_cast is required on the AMD platform (and no checking of upper bits).
There was a problem hiding this comment.
And such a cast would error if information would be lost: https://godbolt.org/z/dj8TKhrnM
That makes sense, thanks!
There was a problem hiding this comment.
@aelovikov-intel Thanks for the approval!
What should I do about the failed workflow? I do not see any relation between the error and my changes, would it make sense to re-run the workflow? Or can the PR still be merged?
| } | ||
| } | ||
| *nativeHandle = static_cast<pi_native_handle>(mem->mem_.buffer_mem_.get()); | ||
| #elif defined(__HIP_PLATFORM_AMD__) |
There was a problem hiding this comment.
And such a cast would error if information would be lost: https://godbolt.org/z/dj8TKhrnM
That makes sense, thanks!
The failure happened on OpenCL and couldn't be affected by the HIP plugin change. I think it could be merged but I don't have write access to the repo. @againull , can you please merge this in? |
This is a fix for #6189.
Replaces invalid
reinterpret_castwithstatic_castforCUdeviceptrinhip_piextMemGetNativeHandleto fix build error occuring when building HIP PI on CUDA machines;Signed-off-by: Lukas Sommer [email protected]