[HIP] We should not be including cmath in our kernels...

[atamazov]

We should not be including cmath in our kernels if we are using hiprtc as all the math function should already be provided by HIP.

If we are doing #include <cmath> in our kernels then this needs to be fixed in either MIOpen or CK.

Originally posted by @pfultz2 in #1921 (comment)

[atamazov]

[Attribution] @junliume https://github.com/ROCmSoftwarePlatform/MIOpen/labels/request_for_comments https://github.com/ROCmSoftwarePlatform/MIOpen/labels/urgency_normal

I recommend assigning @asroy @zjing14 @aska-0096 @ltqin @qianfengz @JehandadKhan @shurale-nkn @carlushuang @pfultz2 for awareness at least.

[atamazov]

@pfultz2 Let me describe the status of cmath usage in our kernels and in CK:

• In some kernels, <cstdint> and <cmath> are used for int8_t, int16_t and float_t. Right now this usage is disabled by workaround for SWDEV-308073 (look up for WORKAROUND_ISSUE_HIPRTC_TRUE_TYPE in Enable HIPRTC support as default from ROCm 5.0 #1237) and things like typedef signed char int8_t are used instead.
  • The workaround is implemented only on COMgr build path (because SWDEV-308073 is related to COMgr). TUNA employs off-line build path for HIP for the sake of speed (@JehandadKhan please note this).
• In CK I see many cases where <cmath> is included, but I do not know if kernels are really using entities from this header (@asroy is it so?) Note that CK is written as "normal" HIP program, i.e. host + device code in the single source, and host code is free to use any C++ features I guess (including <cmath>).

[atamazov]

Another topic. We need to clearly understand what is legal and what is not in HIP language. So far I see the following:

In HIP Kernel Language/C++ Support:

The following C++ features are not supported:

• Run-time-type information (RTTI)
• Virtual functions
• Try/catch

In HIP Kernel Language/Math Functions:

HIP-Clang supports a set of math operations callable from the device...
Following is a table of supported single-precision mathematical functions: ...

CUDA informs about the restriction that standard libs are not supported unless specified otherwise. And there is comprehensive reference on device specific math, which, among other things tells that these functions are always available in device code.

Idealized policy for HIP programs:

• HIP program is not restricted to include any standard header
• The entities from the standard headers should not be used in device code

Let's consider a use case is when both host and device code want to use int8_t. Host code needs <cstdint> for that. However, device code should NOT use int8_t from there. Device code should implement independent definition of int8_t (like typedef signed char), but that would lead to name conflicts. I guess the simplest solution could be enclosing the device code into a namespace, thus isolating standard definitions from their device-specific counterparts.

Ditto all other cases when device code wants to use entities that match the standard ones.

Of course I understand that the above policy is an extreme solution (maybe too extreme) and requires a lot of work.

Guys, what do you think?

[aska-0096]

For the CK backend, Found 7 files including the cmath header, while 5 of them in host code.
When remove the #include<cmath> from device_base.hpp and device_permute.hpp file, CK can pass its compilation on rocm-5.3.0 + develop branch.

[qianfengz]

I will fix the CK including of <cmath> in include/ck/utility/math_v2.hpp . And also remove the <cmath> from device_base.hpp and device_permute.hpp

[qianfengz]

For the CK backend, Found 7 files including the cmath header, while 5 of them in host code. When remove the #include<cmath> from device_base.hpp and device_permute.hpp file, CK can pass its compilation on rocm-5.3.0 + develop branch.

Yes. Including of <cmath> in device_base.hpp and device_permute.hpp is really not needed

[qianfengz]

PR 551 submitted

[pfultz2]

Note that CK is written as "normal" HIP program, i.e. host + device code in the single source, and host code is free to use any C++ features I guess (including ).

Yes, this mainly applies to runtime compilation(ie using hipRTC). Although, CK will be used in MIGraphX as device code, so those changes will eventually need to be made(or at least ifdef the host code).

We need to clearly understand what is legal and what is not in HIP language.

We have other constraints when we are doing runtime compilation(ie hipRTC). We cannot rely on header files being installed, such as from the c++ standard library as those headers are for building packages and not for runtimes.

Device code should implement independent definition of int8_t (like typedef signed char), but that would lead to name conflicts.

hipRTC provides __hip_int8_t types to be used see the documentation here.

I guess the simplest solution could be enclosing the device code into a namespace, thus isolating standard definitions from their device-specific counterparts.

Yes and thats what we are doing in MIGraphX. You can then ifdef the definitions to use std::* types when not using hipRTC.

[atamazov]

@pfultz2

hipRTC provides __hip_int8_t types to be used see the documentation here.

Sure, using entities with different names is another (yet obvious) solution. I meant cases when device code programmer wants (or has) to keep the "original" names ;) And thanks for the link to the deprecation notice.

[atamazov]

@pfultz2 May I ask for advice? Currently:

• In some kernels, <cstdint> and <cmath> are used for int8_t, int16_t and float_t. Right now this usage is disabled by workaround for SWDEV-308073 (look up for WORKAROUND_ISSUE_HIPRTC_TRUE_TYPE in Enable HIPRTC support as default from ROCm 5.0 #1237) and things like typedef signed char int8_t are used instead.

Do you recommend moving from from "workaround" to "standard practice"? The necessary code changes are cosmetic; most likely, it is adequate to simply replace #ifdef WORKAROUND_ISSUE_HIPRTC_TRUE_TYPE with #ifdef __HIPCC_RTC__ in the device code.

[aska-0096]

@atamazov May I close this issue? As CK backend has accepted "remove cmath" PR.

[junliume]

@zjing14 this is the issue I was referring to earlier. Thanks @aska-0096

[atamazov]

@atamazov May I close this issue? As CK backend has accepted "remove cmath" PR.

Basically I do not want this ticket to be ever closed. It is intended to hold the description of the problem, analysis, discussions, and hopefully an agreed decision. That is why it is marked with "RFC" label.

Maybe it is worth converting this to recently introduced "discussion" (if we do not want to use "issues" for sustained topics), but I am still conservative.

[atamazov]

@pfultz2 Can you please let me know your opinion about #1926 (comment)?