[amdgpu][openmp] Treat missing TIMESTAMP_FREQUENCY as non-fatal #70987
Conversation
|
@llvm/pr-subscribers-libc @llvm/pr-subscribers-backend-amdgpu Author: Jon Chesterfield (JonChesterfield) ChangesIf you build with dynamic_hsa, the symbol is known and compilation succeeds. If you then run with a slightly older libhsa, this argument is not recognised and an error returned. I'd rather the program runs with a misleading omp wtime than refuses to run at all. Full diff: https://github.com/llvm/llvm-project/pull/70987.diff 1 Files Affected:
diff --git a/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp b/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
index 71207f767fdcc60..378cad8f8ca4f15 100644
--- a/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
+++ b/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
@@ -1810,10 +1810,12 @@ struct AMDGPUDeviceTy : public GenericDeviceTy, AMDGenericDeviceTy {
return Err;
GridValues.GV_Warp_Size = WavefrontSize;
- // Get the frequency of the steady clock.
- if (auto Err = getDeviceAttr(HSA_AMD_AGENT_INFO_TIMESTAMP_FREQUENCY,
- ClockFrequency))
- return Err;
+ // Get the frequency of the steady clock. If the attribute is missing
+ // assume running on an older libhsa and default to 0, omp_get_wtime
+ // will be inaccurate but otherwise programs can still run.
+ if (auto Err = getDeviceAttrRaw(HSA_AMD_AGENT_INFO_TIMESTAMP_FREQUENCY,
+ ClockFrequency))
+ ClockFrequency = 0;
// Load the grid values dependending on the wavefront.
if (WavefrontSize == 32)
|
| // Get the frequency of the steady clock. If the attribute is missing | ||
| // assume running on an older libhsa and default to 0, omp_get_wtime | ||
| // will be inaccurate but otherwise programs can still run. | ||
| if (auto Err = getDeviceAttrRaw(HSA_AMD_AGENT_INFO_TIMESTAMP_FREQUENCY, |
There was a problem hiding this comment.
So Raw does not emit an error. I thought the issue would be that HSA_AMD_AGENT_INFO_TIMESTAMP_FREQUENCY may not be defined at all so it would fail to compile?
There was a problem hiding this comment.
If the enum field is missing, no compilation, no progress. I don't think there's a reasonable way to reflect on whether an enum field is present or not (i.e. no #ifdef equivalent).
However the enum field is present in the dynamic_hsa header, so openmp compiles just fine against that.
There was a problem hiding this comment.
Was assuming there'd be some version field in HSA that indicates it, but I don't know. This should be better than nothing for now.
There was a problem hiding this comment.
There is no version in the HSA library, it's on the wishlist. There is one in the header, but as the header may bear little relation to the shared library, that is a partial help.
JonChesterfield
force-pushed
the
jc_non_fatal_missing_timestamp
branch
from
37b81fc
to
f81de31
Compare
|
The libc loader already treats the symbol on the GPU as optional (since it might be deadstripped), this patch extends libc to likewise use zero for the frequency if the capability is missing from hsa instead of refusing to run |
If you build with dynamic_hsa, the symbol is known and compilation succeeds. If you then run with a slightly older libhsa, this argument is not recognised and an error returned. I'd rather the program runs with a misleading omp wtime than refuses to run at all.