[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.


================
Comment at: lib/Driver/ToolChain.cpp:808
+          continue;
+      } else if (XOpenMPTargetNoTriple)
+        // Passing device args: -Xopenmp-target -opt=val.
----------------
Please include {} around this else-if  code, even though it is not necessary, because the other blocks require it.


================
Comment at: lib/Driver/ToolChain.cpp:820
+      if (!XOpenMPTargetArg || Index > Prev + 1) {
+        getDriver().Diag(diag::err_drv_invalid_Xopenmp_target_with_args)
+            << A->getAsString(Args);
----------------
Is this covered by a test case?


================
Comment at: lib/Driver/ToolChain.cpp:827
+              options::OPT_fopenmp_targets_EQ).size() != 1) {
+        getDriver().Diag(diag::err_drv_Xopenmp_target_missing_triple);
+        continue;
----------------
Is this covered by a test case?


================
Comment at: lib/Driver/ToolChain.cpp:834
+      // Ignore all but last -march=arch flag.
+      if (A->getOption().matches(options::OPT_march_EQ))
+        DAL->eraseArg(options::OPT_march_EQ);
----------------
Why is `-march` special in this regard? Shouldn't the consumers just take the last one specified (e.g., use getLastArgValue in the ToolChain code)?


================
Comment at: test/Driver/openmp-offload.c:615
+
+// CHK-FOPENMP-TARGET: clang{{.*}} argument unused during compilation: '-Xopenmp-target -march=pwr8'
----------------
Now that this is in common code, why are these arguments still unused?


https://reviews.llvm.org/D34784