tra added inline comments.

================
Comment at: clang/include/clang/Basic/Cuda.h:105-107
+constexpr CudaArch DefaultCudaArch = CudaArch::SM_35;
+constexpr CudaArch DefaultHIPArch = CudaArch::GFX803;
+
----------------
Nit: those could be just enum values.
```
  ...
  LAST,
  DefaultCudaArch = SM_35,
  DefaultHIPArch = GFX803,
};
```


================
Comment at: clang/lib/Driver/Driver.cpp:4219
+    for (unsigned I = 0, E = TCAndArchs.size(); I != E; ++I)
       DeviceActions.push_back(C.MakeAction<InputAction>(*InputArg, InputType));
 
----------------
This loop can be folded into the loop above.

Alternatively, for simple loops like this one could use `llvm::for_each`.


================
Comment at: clang/lib/Driver/Driver.cpp:4244
           A = C.MakeAction<OffloadAction>(HDep, DDep);
+          ++TCAndArch;
+        } else if (isa<AssembleJobAction>(A) && Kind == Action::OFK_Cuda) {
----------------
Could you elaborate why TCAndArch is incremented only here? Don't other cases 
need to advance it, too? At the very least we need some comments here and for 
the loop in general, describing what's going on.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6998
+      // The CUDA toolchain should have a bound arch appended to the filename.
+      StringRef Arch = File.rsplit(".").first.rsplit('-').second;
+      CmdArgs.push_back(Args.MakeArgString(
----------------
Extracting arch name from the file name looks... questionable. Where does that 
file name come from? Can't we get this information directly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120272

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to