jhuber6 added inline comments.
================
Comment at: clang/include/clang/Basic/Cuda.h:105-107
+constexpr CudaArch DefaultCudaArch = CudaArch::SM_35;
+constexpr CudaArch DefaultHIPArch = CudaArch::GFX803;
+
----------------
tra wrote:
> Nit: those could be just enum values.
> ```
> ...
> LAST,
> DefaultCudaArch = SM_35,
> DefaultHIPArch = GFX803,
> };
> ```
Will do.
================
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));
----------------
tra wrote:
> This loop can be folded into the loop above.
>
> Alternatively, for simple loops like this one could use `llvm::for_each`.
It could, I think the intention is clearer (i.e. make an input for every
toolchain and architecture) having them separate. But I can merge them if
that's better.
================
Comment at: clang/lib/Driver/Driver.cpp:4244
A = C.MakeAction<OffloadAction>(HDep, DDep);
+ ++TCAndArch;
+ } else if (isa<AssembleJobAction>(A) && Kind == Action::OFK_Cuda) {
----------------
tra wrote:
> 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.
It shouldn't be, I forgot to move this out of the conditional once I added more
things, I'll explain the usage as well.
================
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(
----------------
tra wrote:
> Extracting arch name from the file name looks... questionable. Where does
> that file name come from? Can't we get this information directly?
Yeah, it's not ideal but it was the easiest way to do it. The alternative is to
find a way to traverse the job action list and find the nodes with a bound
architecture set and hope they're in the same order. I can try to do that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120272/new/
https://reviews.llvm.org/D120272
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
