Remove the dependence on standard C++ header
for overloaded math functions in HIP header
since standard C++ header is not available for hipRTC.
Details
- Reviewers
tra - Commits
- rG8baba6890de7: [HIP] Support overloaded math functions for hipRTC
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM overall.
@jlebar: I could use your opinion here.
clang/lib/Headers/__clang_hip_cmath.h | ||
---|---|---|
343–344 | @jlebar : Should we expect any observable surprises here? | |
clang/lib/Headers/__clang_hip_runtime_wrapper.h | ||
76–86 | I'd add an empty line separators between #if/#endif blocks. Or, perhaps, it should be restructured as #if defined(__HIPCC_RTC__) #include <__clang_hip_cmath.h> #else #include <__clang_cuda_math_forward_declares.h> #include <__clang_hip_cmath.h> #include <__clang_cuda_complex_builtins.h> #include <algorithm> #include <complex> #include <new> #endif // __HIPCC_RTC__ |
clang/lib/Headers/__clang_hip_runtime_wrapper.h | ||
---|---|---|
76–86 | will restructure it |
clang/lib/Headers/__clang_hip_cmath.h | ||
---|---|---|
343–344 | _Tp{} may break if _Tp does not have a default constructor. At least it does so with C++20, while the old code did not: https://godbolt.org/z/PGEff5Mvv |
clang/lib/Headers/__clang_hip_cmath.h | ||
---|---|---|
343–344 | Right. It occurred to me that users may overload these math functions with arguments without default ctor, then we will hit the issue. I will fix this. |
Other than the declval issue discussed above, this looks reasonable to me. I don't notice anything wrong with the standard library reimplementations here.
clang/lib/Headers/__clang_hip_cmath.h | ||
---|---|---|
345 | will do |
clang/lib/Headers/__clang_hip_cmath.h | ||
---|---|---|
345 | actually this is end of struct __numeric_type. end of namespace __hip is at line 390 |
clang/lib/Headers/__clang_hip_cmath.h | ||
---|---|---|
309 | This changes the overloading behavior of math functions with _Float16 arguments since previously we use std::numeric_limits<T>::is_specialized to determine whether a type is an arithmetic, which is not specialized for _Float16. To make this patch NFC for non-hipRTC, I would like to split this part to another patch. This facilitates debugging if issues arise |
clang/lib/Headers/__clang_hip_cmath.h | ||
---|---|---|
586–587 | Nit: I'd add // <condition> here for consistency. |
we may not need to #include limits and/or type_traits if we replaced them with __hip::is_integral and __hip::is_arithmetic.