This patch is suppose to fix the issue of hsa.h not found.
Issue was reported in D99949
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Logic doesn't look quite right to me. If the compiler supports has_include, but neither of those headers exist, we fall through to main() which won't compile.
How about:
#if defined(__has_include) #if __has_include("hsa.h") #define HSA_FOUND 1 // name tbd #include "hsa.h" #elif __has_include("hsa/hsa.h") #define HSA_FOUND 1 #include "hsa/hsa.h" #else #define HSA_FOUND 0 #endif #else #define HSA_FOUND 0 #endif #if !HSA_FOUND int main() { return 1; } #else // all the current stuff #endif
That takes the approach that missing __has_include is a bad sign and immediately gives up. As far as I can tell it was introduced ~ 10 years ago, and was more recently added to c++17 (https://en.cppreference.com/w/cpp/preprocessor/include, which uses a similar pattern to the above in terms of booleans). Seems to be present in gcc, clang, msvc and we have a safe fallback if it isn't.
edit: We could also start down the dlopen path, but let's not do that if we can avoid it
LGTM, thanks. I think this leaves us totally sure the clang build will succeed with a best effort chance of having a working amdgpu ident tool afterwards.
A thought - we need to keep return 0 for success, but could return a different integer for each failure mode. That would be useful when guessing why it failed. Orthogonal to this patch.
clang-tidy: error: unknown type name 'hsa_status_t' [clang-diagnostic-error]
not useful
clang-tidy: error: unknown type name 'hsa_agent_t' [clang-diagnostic-error]
not useful