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

Artem Belevich via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 5 16:10:23 PDT 2022


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



More information about the cfe-commits mailing list