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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
