[PATCH] D61399: [OpenMP][Clang] Support for target math functions

Thu May 2 10:29:32 PDT 2019

hfinkel accepted this revision.
hfinkel added inline comments.
This revision is now accepted and ready to land.

================
Comment at: lib/Driver/ToolChains/Clang.cpp:1163
+      llvm::sys::path::append(P, "openmp_wrappers");
+      CmdArgs.push_back("-internal-isystem");
+      CmdArgs.push_back(Args.MakeArgString(P));
----------------
Please add a driver test covering this.

================
Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math.h:20
+/// respectively. This is actually what the Clang-CUDA code path does, using
+/// __device__ instead of variants to avoid redeclarations and get the desired
+/// overload resolution.
----------------
And this is why I wanted to just make `__device__` work in OpenMP mode. But, as a temporary solution until we get `declare variant` working, I'm okay with this.

Modulo the naming nit and the missing driver test, LGTM.

================
Comment at: lib/Headers/openmp_wrappers/__clang_openmp_math.h:46
+/// Magic macro for stopping the math.h/cmath host header from being included.
+#define __NO_HOST_MATH__
+
----------------
I'd prefer that, as this is clang specific, we make this clear by naming this `__CLANG_NO_HOST_MATH__`.

Repository:
  rC Clang

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

https://reviews.llvm.org/D61399