Page MenuHomePhabricator

[HIP] Support overloaded math functions for hipRTC
ClosedPublic

Authored by yaxunl on Apr 19 2021, 2:20 PM.

Details

Summary

Remove the dependence on standard C++ header
for overloaded math functions in HIP header
since standard C++ header is not available for hipRTC.

Diff Detail

Event Timeline

yaxunl requested review of this revision.Apr 19 2021, 2:20 PM
yaxunl created this revision.
tra added a subscriber: jlebar.Apr 19 2021, 2:58 PM

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__
yaxunl marked an inline comment as done.Apr 20 2021, 7:07 AM
yaxunl added inline comments.
clang/lib/Headers/__clang_hip_runtime_wrapper.h
76–86

will restructure it

yaxunl updated this revision to Diff 338864.Apr 20 2021, 7:31 AM
yaxunl marked an inline comment as done.

revised by Artem's comments

tra added inline comments.Apr 20 2021, 11:43 AM
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

yaxunl marked 2 inline comments as done.Apr 20 2021, 12:03 PM
yaxunl added inline comments.
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.

yaxunl updated this revision to Diff 339009.Apr 20 2021, 3:18 PM
yaxunl marked an inline comment as done.

fix hip::numeric_type

tra accepted this revision.Apr 20 2021, 4:27 PM

LGTM.

clang/lib/Headers/__clang_hip_cmath.h
345

Nit: }; // namespace __hip

This revision is now accepted and ready to land.Apr 20 2021, 4:27 PM
yaxunl marked an inline comment as done.Apr 20 2021, 8:01 PM
yaxunl added inline comments.
clang/lib/Headers/__clang_hip_cmath.h
345

will do

yaxunl marked an inline comment as done.Apr 20 2021, 8:05 PM
yaxunl added inline comments.
clang/lib/Headers/__clang_hip_cmath.h
345

actually this is end of struct __numeric_type. end of namespace __hip is at line 390

yaxunl added inline comments.Apr 22 2021, 12:33 PM
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

yaxunl updated this revision to Diff 339773.Apr 22 2021, 1:47 PM

make it NFC for non-hipRTC

tra accepted this revision.Apr 22 2021, 2:53 PM
tra added inline comments.
clang/lib/Headers/__clang_hip_cmath.h
586–587

Nit: I'd add // <condition> here for consistency.

This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 4:07 PM
ashi1 added a subscriber: ashi1.Fri, May 21, 9:09 AM
ashi1 added inline comments.
clang/lib/Headers/__clang_hip_cmath.h
20

we may not need to #include limits and/or type_traits if we replaced them with __hip::is_integral and __hip::is_arithmetic.

587

I don't think this #endif is for !defined(__HIPCC_RTC__). It should be for _LIBCPP_BEGIN_NAMESPACE_STD.

yaxunl marked 2 inline comments as done.Fri, May 21, 2:47 PM
yaxunl added inline comments.
clang/lib/Headers/__clang_hip_cmath.h
20

right. but we may need to move them to HIP headers first if HIP headers need them.

587

You are right. fixed.