[PATCH] D29647: [OpenMP] Extend CLANG target options with device offloading kind.

[Justin Lebar via Phabricator via cfe-commits]

jlebar added inline comments.


================
Comment at: lib/Driver/ToolChains.cpp:4902
+          DeviceOffloadingKind == Action::OFK_Cuda) &&
+         "The offloading kind is not OpenMP or CUDA.");
 
----------------
Not sure this assertion message helps us much beyond what's already in the code.


================
Comment at: lib/Driver/ToolChains.cpp:4914
+                           options::OPT_fno_cuda_approx_transcendentals,
+                           false))
+      CC1Args.push_back("-fcuda-approx-transcendentals");
----------------
Are these changes related to this patch?

I have no problem cleaning up whitespace errors like these, but would prefer for them to be split out separately if possible.


================
Comment at: lib/Driver/ToolChains.cpp:4961
 
+  // If this is an OpenMP device we only need to append the gpu name.
+  if (DeviceOffloadKind == Action::OFK_OpenMP) {
----------------
s/device/compilation/?


================
Comment at: lib/Driver/ToolChains.cpp:4961
 
+  // If this is an OpenMP device we only need to append the gpu name.
+  if (DeviceOffloadKind == Action::OFK_OpenMP) {
----------------
jlebar wrote:
> s/device/compilation/?
An "otherwise" would probably be helpful in this comment.


================
Comment at: lib/Driver/ToolChains.cpp:4966
+
+    // TO DO: get the right arch from the offloading arguments when not
+    // using the default compute capability of sm_20.
----------------
TODO is idiomatically one word


================
Comment at: lib/Driver/Tools.cpp:12136
   // Obtain architecture from the action.
-  CudaArch gpu_arch = StringToCudaArch(JA.getOffloadingArch());
   assert(gpu_arch != CudaArch::UNKNOWN &&
----------------
Why does JA.getOffloadingArch have the wrong value?  Isn't the purpose of JA.getOffloadingArch to have this particular value?  If it's broken, can we fix it instead of doing this hack?


Repository:
  rL LLVM

https://reviews.llvm.org/D29647