[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Really looking forward to this! Thanks a lot!

I left some comments.



================
Comment at: clang/lib/Headers/__clang_hip_math.h:35
+#ifdef __OPENMP_AMDGCN__
+#define __RETURN_TYPE int
+#else
----------------
JonChesterfield wrote:
> I'd expect openmp to match the cplusplus/c distinction here, as openmp works on C source
^ Agreed. Though, we use a different trick because it's unfortunately not as straight forward always and can be decided based on the C vs C++.


================
Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h:112
+
+#pragma omp end declare variant
+
----------------
Can you make the declare variant scope of nvptx and amdgpu smaller and put them next to each other.

```

#ifdef __cplusplus
extern "C" {
#endif

#declare variant
#define ...
...
#undef
#end


#declare variant
...
#end

#ifdef __cplusplus
} // extern "C"


```


================
Comment at: clang/lib/Headers/openmp_wrappers/cmath:83
+#include <__clang_hip_cmath.h>
+#undef __OPENMP_AMDGCN__
+
----------------
No match_any needed (here and elsewhere).

Also, don't we want all but the includes to be the same for both GPUs. Maybe we have a device(kind(gpu)) variant and inside the nvptx and amdgpu just for the respective include?


================
Comment at: clang/lib/Headers/openmp_wrappers/math.h:59
+#pragma omp end declare variant
+
 #endif
----------------
FWIW, This is what I think the begin/end regions should look like. Small and next to each other.


================
Comment at: clang/test/Headers/Inputs/include/cstdlib:15
 
+#ifndef __AMDGCN__
 namespace std
----------------
JonChesterfield wrote:
> I think I'd expect builtin_labs et al to work on amdgcn, are we missing lowering for them?
Yeah, looks weird that we cannot compile this mock-up header.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104904/new/

https://reviews.llvm.org/D104904