[PATCH] D125050: [OpenMP] Try to Infer target triples using the offloading architecture

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Just a drive-by style review. I didn't look at the functionality of the changes much.



================
Comment at: clang/lib/Driver/Driver.cpp:788-789
+                               options::OPT_fno_openmp, false) &&
+      (C.getInputArgs().hasArg(options::OPT_fopenmp_targets_EQ) ||
+       C.getInputArgs().hasArg(options::OPT_offload_arch_EQ));
+  if (IsOpenMP) {
----------------
So, specifying just `-fopenmp` will result in `IsOpenMP=false`? This seems odd.
Perhaps the variable should be called `IsOpenMPOffload` ? 


================
Comment at: clang/lib/Driver/Driver.cpp:821-822
+                                                      HostTC->getTriple());
+      if (!AMDTriple || !NVPTXTriple) {
+        Diag(clang::diag::err_drv_failed_to_deduce_targets);
+        return;
----------------
This seems a bit too restrictive as it would fail if we've failed to get either of those triples. We may be able to proceed with only one of them in many cases.
I'd remove this check and would make the loop below more forgiving. 


================
Comment at: clang/lib/Driver/Driver.cpp:831
+      for (StringRef Arch : Archs) {
+        if (IsNVIDIAGpuArch(StringToCudaArch(
+                getProcessorFromTargetID(*NVPTXTriple, Arch)))) {
----------------
```
if (NvidiaTriple && IsNVIDIAGpuArch(...))
...
else if (AMDTriple && IsAMDGpuArch(...))
...
else {
   Diag(); 
   return;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125050