[PATCH] D60907: [OpenMP] Add math functions support in OpenMP offloading

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

+1 to Hal's comments.

@jdoerfert :

> I'd even go as far as to argue that __clang_cuda_device_functions.h should include the internal math.h wrapper to get all math functions. See also the next comment.

I'd argue other way around -- include __clang_cuda_device_functions.h from math.h and do not preinclude anything.
If the user does not include math.h, it should not have its namespace polluted by some random stuff. NVCC did this, but that's one of the most annoying 'features' we have to be compatible with for the sake of keeping existing nvcc-compilable CUDA code happy.

If users do include math.h, it should do the right thing, for both sides of the compilation.
IMO It's math.h that should be triggering pulling device functions in.



================
Comment at: lib/Driver/ToolChains/Clang.cpp:1157-1159
+  if (JA.isDeviceOffloading(Action::OFK_OpenMP) &&
+      getToolChain().getTriple().isNVPTX())
+    getToolChain().AddMathDeviceFunctions(Args, CmdArgs);
----------------
This functionality is openMP-specific, but the function name `AddMathDeviceFunctions()` is not. I'd rather keep OpenMP specialization down where it can be easily seen. Could this check be pushed down into CudaInstallationDetector::AddMathDeviceFunctions() ?

Another potential problem here is that this file will be pre-included only for the device. It will potentially result in more observable semantic differences between host and device compilations. I don't know if it matters for OpenMP, though.

IMO intercepting math.h and providing device-specific overloads *in addition* to the regular math.h functions would be a better approach.

Another problem with pre-included files is that sometimes users may intentionally need to *not* include them.
For CUDA we have `-nocudainc` flag. Your change, at least, will need something similar, IMO.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60907