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

[Aaron Enye Shi via Phabricator via cfe-commits]

ashi1 added a comment.

A few small comments, otherwise LGTM on the HIP header side.



================
Comment at: clang/lib/Headers/__clang_hip_cmath.h:30
+#ifdef __OPENMP_AMDGCN__
+#define __DEVICE__ static __attribute__((always_inline, nothrow))
+#define __CONSTEXPR__ constexpr
----------------
Does OpenMP not require `__device__` attribute here? I know constexpr defines `__device__` on HIP, does OMP do the same?


================
Comment at: clang/lib/Headers/__clang_hip_cmath.h:32
+#define __CONSTEXPR__ constexpr
+#define __constant__ __attribute__((constant))
+#else
----------------
I don't think this is the right place to define `__constant__`? It's unused in this header, and may get forgotten. Would it be better to define it in the openmp wrapper or does cmath define it in OpenMP?


================
Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h:47
+#define __OPENMP_AMDGCN__
+#define __device__
+/// Include declarations for libdevice functions.
----------------
Would it be better to push and pop these macros, in case it was defined outside of here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104904