[SYCL][HIP] Add cl_khr_fp64 in device extensions
#7428
Conversation
sycl/plugins/hip/pi_hip.cpp
Outdated
| @@ -1655,7 +1655,7 @@ pi_result hip_piDeviceGetInfo(pi_device device, pi_device_info param_name, | |||
| // DEVICELIB_ASSERT extension is set so fallback assert | |||
| // postprocessing is NOP. HIP 4.3 docs indicate support for | |||
| // native asserts are in progress | |||
| std::string SupportedExtensions = ""; | |||
| std::string SupportedExtensions = "cl_khr_fp64 "; | |||
There was a problem hiding this comment.
Could you add a test to llvm-test-suite or if you can make it compiler only add it to the compiler only tests in this repo? This would stop the issue from coming back.
There was a problem hiding this comment.
Do all existing HIP devices support cl_khr_fp64 functionality?
There was a problem hiding this comment.
I have done some digging, I cannot find any indication that different devices have different precision support. So, I would assume all HIP devices support fp64
There was a problem hiding this comment.
The oldest device of HIP I found seems to be gfx803, which has fp64 capability. (Although support of gfx803 and gfx900 has been dropped.)
And the CUDA plugin has already added cl_khr_fp64 here, so I assumed all devices supported by HIP have fp64.
There was a problem hiding this comment.
Could you add a test to llvm-test-suite or if you can make it compiler only add it to the compiler only tests in this repo? This would stop the issue from coming back.
Sorry I'm not familiar with LLVM things, I'm not sure how to do that... Is a device.has(aspect::fp64) enough?
The commit that added cl_khr_fp64 for CUDA plugin doesn't have a test on cl_khr_fp64.
And I've also searched similar tests on cl_khr_fp64 among CUDA plugin's tests for reference, but didn't find one...
There was a problem hiding this comment.
So from looking into HIP further it seems like you can query from the host if the device supports fp64.
https://github.com/ROCm-Developer-Tools/HIP/blob/0acac9c7db45c8e3c107198eebfa83cb031715c4/include/hip/hip_runtime_api.h#L58
You can use hipGetDeviceProperties to get the hipDeviceProp_t and in this struct there is the hipDeviceArch_t.
So perhaps this is what we should do rather than assume the device supports fp64. Also now as it is clear the device cannot be guaranteed to have fp64 then I don't see a way of writing a valid test, so it is fine that we don't add one along with this PR.
|
Thanks @AidanBeltonS ! Could you please take a look now ? |
|
Test failure seems unrelated and comes from a test change, see intel/llvm-test-suite#1380 |
|
LGTM |
HIP plugin didn't have
cl_khr_fp64in SupportedExtensions, so any kernel uses fp64 cannot be launched becauseProgramManager::getBuiltPIProgramcheckshas(aspect::fp64)which is actuallyhas_extension("cl_khr_fp64").