tra added inline comments.
================
Comment at: clang/lib/Driver/Driver.cpp:4107
+ options::OPT_no_offload_arch_EQ)) {
+ C.getDriver().Diag(diag::err_opt_not_valid_with_opt) << "--offload-arch"
+ << "--offload";
----------------
Nit: It would be nice to report specific option which triggered the diag.
Reporting `--offload-arch` when it's triggered by `--no-offload-arch` would be
somewhat confusing.
`Args.hasArgNoClaim(options::OPT_offload_arch_EQ) ? "--offload-arch" :
--no-offload-arch` ?
================
Comment at: clang/lib/Driver/Driver.cpp:4132-4134
+ Archs.insert(CudaArchToString(CudaArch::SM_35));
+ else if (Kind == Action::OFK_HIP)
+ Archs.insert(CudaArchToString(CudaArch::GFX803));
----------------
If we do not have constants for the default CUDA/HIP arch yet, we should
probably add them and use them here.
================
Comment at: clang/lib/Driver/Driver.cpp:4171
- for (unsigned I = 0; I < ToolChains.size(); ++I)
+ if (!Relocatable)
+ Diags.Report(diag::err_drv_non_relocatable);
----------------
If we do not allow non-relocatable compilation with the new driver, perhaps we
should make `-fgpu-rdc` enabled by default with the new driver and error out if
someone attempts to use `-fno-gpu-rdc`. Otherwise we're virtually guaranteed
that everyone attempting to use `-foffload-new-driver` will hit this error.
================
Comment at: clang/lib/Driver/Driver.cpp:4172
+ if (!Relocatable)
+ Diags.Report(diag::err_drv_non_relocatable);
+
----------------
Do we want to return early here?
================
Comment at: clang/lib/Driver/Driver.cpp:4221
+ auto TCAndArch = TCAndArchs.begin();
for (Action *A : DeviceActions) {
+ DDeps.add(*A, *TCAndArch->first, TCAndArch->second.data(), Kind);
----------------
Nit: We do have `llvm::zip` for iterating over multiple containers. However,
unpacking loop variable results maybe more trouble than it's worth it in such a
small loop, Up to you.
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
