https://github.com/arsenm updated https://github.com/llvm/llvm-project/pull/198627
>From 7d4122a69e8a67be4fe5ccff0cb889cfbce091e5 Mon Sep 17 00:00:00 2001 From: Matt Arsenault <[email protected]> Date: Thu, 7 May 2026 16:08:42 +0100 Subject: [PATCH] clang/AMDGPU: Use TranslateArgs from the base toolchain instead of the host This fixes -Xopenmp-target / -Xarch for arbitrary arguments. HIP and OpenMP had cargo-cult broken implementations of TranslateArgs, which called the host toolchain's implementation, and then special case transferred either -march or -mcpu to the device argument list. The respective device forwarding flags should work for any argument, not just this one. The main feature that needs to be preserved is the shared filtering of unsupported sanitizers to degrade them into warnings. Most of the changes here are dealing with fallout observed when the host target is darwin. The darwin toolchain happens to have some hacky statefulness tracking the compile target version, which gets written and rewritten on argument parsing. To maintain this hack, there are a few unused calls to getArgsForToolChain; start passing OFK_Host to these so the offload toolchains don't get confused and think they're in a non-offload context. --- clang/lib/Driver/Driver.cpp | 4 +- clang/lib/Driver/ToolChain.cpp | 15 ++++- clang/lib/Driver/ToolChains/AMDGPU.cpp | 60 ++++++++++++++----- clang/lib/Driver/ToolChains/AMDGPU.h | 30 ++-------- clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp | 42 ------------- clang/lib/Driver/ToolChains/AMDGPUOpenMP.h | 5 -- clang/lib/Driver/ToolChains/Gnu.cpp | 23 ++++--- clang/lib/Driver/ToolChains/HIPAMD.cpp | 40 ++----------- clang/lib/Driver/ToolChains/HIPAMD.h | 1 - clang/test/Driver/offload-darwin-host.hip | 8 +++ .../openmp-Xopenmp-target-forward-args.c | 10 ++++ 11 files changed, 102 insertions(+), 136 deletions(-) create mode 100644 clang/test/Driver/offload-darwin-host.hip create mode 100644 clang/test/Driver/openmp-Xopenmp-target-forward-args.c diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp index 4a968a4ce5cc0..7ee4163030a5e 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -2709,7 +2709,7 @@ bool Driver::HandleImmediateArgs(Compilation &C) { // FIXME: Remove when darwin's toolchain is initialized during construction. // FIXME: For some more esoteric targets the default toolchain is not the // correct one. - C.getArgsForToolChain(&TC, Triple.getArchName(), Action::OFK_None); + C.getArgsForToolChain(&TC, Triple.getArchName(), Action::OFK_Host); RegisterEffectiveTriple TripleRAII(TC, Triple); switch (RLT) { case ToolChain::RLT_CompilerRT: @@ -6084,7 +6084,7 @@ InputInfoList Driver::BuildJobsForActionNoCache( // computed. Get the default arguments for OFK_None to ensure that // initialization is performed before processing the offload action. // FIXME: Remove when darwin's toolchain is initialized during construction. - C.getArgsForToolChain(TC, BoundArch, Action::OFK_None); + C.getArgsForToolChain(TC, BoundArch, Action::OFK_Host); // The offload action is expected to be used in four different situations. // diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp index 21d61a60224cb..95413c57e7c06 100644 --- a/clang/lib/Driver/ToolChain.cpp +++ b/clang/lib/Driver/ToolChain.cpp @@ -519,11 +519,20 @@ SanitizerArgs ToolChain::getSanitizerArgs(const llvm::opt::ArgList &JobArgs, StringRef BoundArch, Action::OffloadKind DeviceOffloadKind) const { + // When -fno-gpu-sanitize is specified for GPU targets, don't emit + // diagnostics about unsupported sanitizers for specific GPU arches, + // since sanitizers are disabled for the GPU anyway. + bool DiagnoseBoundArchErrors = + BoundArchSanitizerArgsChecked.insert(BoundArch).second; + if (!BoundArch.empty() && getTriple().isGPU() && + !JobArgs.hasFlag(options::OPT_fgpu_sanitize, + options::OPT_fno_gpu_sanitize, true)) { + DiagnoseBoundArchErrors = false; + } + SanitizerArgs SanArgs(*this, JobArgs, /*DiagnoseErrors=*/!SanitizerArgsChecked, - /*DiagnoseBoundArchErrors=*/ - BoundArchSanitizerArgsChecked.insert(BoundArch).second, - BoundArch, DeviceOffloadKind); + DiagnoseBoundArchErrors, BoundArch, DeviceOffloadKind); SanitizerArgsChecked = true; return SanArgs; diff --git a/clang/lib/Driver/ToolChains/AMDGPU.cpp b/clang/lib/Driver/ToolChains/AMDGPU.cpp index c58891b1add56..97aad4f4ca19b 100644 --- a/clang/lib/Driver/ToolChains/AMDGPU.cpp +++ b/clang/lib/Driver/ToolChains/AMDGPU.cpp @@ -713,23 +713,24 @@ Tool *AMDGPUToolChain::buildLinker() const { DerivedArgList * AMDGPUToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch, Action::OffloadKind DeviceOffloadKind) const { - DerivedArgList *DAL = Generic_ELF::TranslateArgs(Args, BoundArch, DeviceOffloadKind); - - const OptTable &Opts = getDriver().getOpts(); - - if (!DAL) + if (!DAL) { DAL = new DerivedArgList(Args.getBaseArgs()); + for (Arg *A : Args) + DAL->append(A); + } - for (Arg *A : Args) - DAL->append(A); + const OptTable &Opts = getDriver().getOpts(); - // AMDGPU is intended to use `-mcpu` but we accept `-march` for legacy. - if (Arg *A = DAL->getLastArg(options::OPT_march_EQ)) { - DAL->eraseArg(options::OPT_march_EQ); - if (!DAL->hasArg(options::OPT_mcpu_EQ)) - DAL->AddJoinedArg(A, Opts.getOption(options::OPT_mcpu_EQ), A->getValue()); + if (DeviceOffloadKind == Action::OFK_None) { + // AMDGPU is intended to use `-mcpu` but we accept `-march` for legacy. + if (Arg *A = DAL->getLastArg(options::OPT_march_EQ)) { + DAL->eraseArg(options::OPT_march_EQ); + if (!DAL->hasArg(options::OPT_mcpu_EQ)) + DAL->AddJoinedArg(A, Opts.getOption(options::OPT_mcpu_EQ), + A->getValue()); + } } // Replace -mcpu=native with detected GPU. @@ -750,7 +751,13 @@ AMDGPUToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch, } } - checkTargetID(*DAL); + if (!BoundArch.empty()) { + DAL->eraseArg(options::OPT_mcpu_EQ); + DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_mcpu_EQ), BoundArch); + checkTargetID(*DAL); + } else if (DeviceOffloadKind == Action::OFK_None) { + checkTargetID(*DAL); + } if (Args.getLastArgValue(options::OPT_x) != "cl") return DAL; @@ -837,6 +844,29 @@ ROCMToolChain::ROCMToolChain(const Driver &D, const llvm::Triple &Triple, RocmInstallation->detectDeviceLibrary(); } +DerivedArgList * +ROCMToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch, + Action::OffloadKind DeviceOffloadKind) const { + DerivedArgList *DAL = + AMDGPUToolChain::TranslateArgs(Args, BoundArch, DeviceOffloadKind); + + // Filter out sanitizer coverage options that are not supported for AMDGPU. + for (Arg *A : Args) { + // Sanitizer coverage is currently not supported for AMDGPU. + if (A->getOption().matches(options::OPT_fsan_cov_Group)) { + // Upgrade to error if the option was explicitly specified for device + bool IsExplicitDevice = + A->getBaseArg().getOption().matches(options::OPT_Xarch_device); + getDriver().Diag(IsExplicitDevice + ? diag::err_drv_unsupported_option_for_target + : diag::warn_drv_unsupported_option_for_target) + << A->getAsString(Args) << getTriple().str(); + } + } + + return DAL; +} + void AMDGPUToolChain::addClangTargetOptions( const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args, @@ -906,8 +936,8 @@ AMDGPUToolChain::getGPUArch(const llvm::opt::ArgList &DriverArgs) const { AMDGPUToolChain::ParsedTargetIDType AMDGPUToolChain::getParsedTargetID(const llvm::opt::ArgList &DriverArgs) const { StringRef TargetID = DriverArgs.getLastArgValue(options::OPT_mcpu_EQ); - if (TargetID.empty()) - TargetID = DriverArgs.getLastArgValue(options::OPT_march_EQ); + //if (TargetID.empty()) + //TargetID = DriverArgs.getLastArgValue(options::OPT_march_EQ); if (TargetID.empty()) return {std::nullopt, std::nullopt, std::nullopt}; diff --git a/clang/lib/Driver/ToolChains/AMDGPU.h b/clang/lib/Driver/ToolChains/AMDGPU.h index 3d291e9e08cb1..b87cefdcaaf38 100644 --- a/clang/lib/Driver/ToolChains/AMDGPU.h +++ b/clang/lib/Driver/ToolChains/AMDGPU.h @@ -141,6 +141,11 @@ class LLVM_LIBRARY_VISIBILITY ROCMToolChain : public AMDGPUToolChain { public: ROCMToolChain(const Driver &D, const llvm::Triple &Triple, const llvm::opt::ArgList &Args); + + llvm::opt::DerivedArgList * + TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef BoundArch, + Action::OffloadKind DeviceOffloadKind) const override; + void addClangTargetOptions(const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args, @@ -151,31 +156,6 @@ class LLVM_LIBRARY_VISIBILITY ROCMToolChain : public AMDGPUToolChain { getCommonDeviceLibNames(const llvm::opt::ArgList &DriverArgs, llvm::StringRef TargetID, llvm::StringRef GPUArch, Action::OffloadKind DeviceOffloadingKind) const; - - bool diagnoseUnsupportedOption(const llvm::opt::Arg *A, - const llvm::opt::DerivedArgList &DAL, - const llvm::opt::ArgList &DriverArgs, - const char *Value = nullptr) const { - auto &Diags = getDriver().getDiags(); - bool IsExplicitDevice = - A->getBaseArg().getOption().matches(options::OPT_Xarch_device); - - if (Value) { - unsigned DiagID = - IsExplicitDevice - ? clang::diag::err_drv_unsupported_option_part_for_target - : clang::diag::warn_drv_unsupported_option_part_for_target; - Diags.Report(DiagID) << Value << A->getAsString(DriverArgs) - << getTriple().str(); - } else { - unsigned DiagID = - IsExplicitDevice - ? clang::diag::err_drv_unsupported_option_for_target - : clang::diag::warn_drv_unsupported_option_for_target; - Diags.Report(DiagID) << A->getAsString(DAL) << getTriple().str(); - } - return true; - } }; } // end namespace toolchains diff --git a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp index aecec6c4c419b..1303e4e0e6799 100644 --- a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp +++ b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp @@ -33,8 +33,6 @@ AMDGPUOpenMPToolChain::AMDGPUOpenMPToolChain(const Driver &D, void AMDGPUOpenMPToolChain::addClangTargetOptions( const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args, Action::OffloadKind DeviceOffloadingKind) const { - HostTC.addClangTargetOptions(DriverArgs, CC1Args, DeviceOffloadingKind); - assert(DeviceOffloadingKind == Action::OFK_OpenMP && "Only OpenMP offloading kinds are supported."); @@ -53,46 +51,6 @@ void AMDGPUOpenMPToolChain::addClangTargetOptions( return; } -llvm::opt::DerivedArgList *AMDGPUOpenMPToolChain::TranslateArgs( - const llvm::opt::DerivedArgList &Args, StringRef BoundArch, - Action::OffloadKind DeviceOffloadKind) const { - DerivedArgList *DAL = - HostTC.TranslateArgs(Args, BoundArch, DeviceOffloadKind); - - if (!DAL) - DAL = new DerivedArgList(Args.getBaseArgs()); - - const OptTable &Opts = getDriver().getOpts(); - - for (Arg *A : Args) { - // Sanitizer coverage is currently not supported for AMDGPU. - if (A->getOption().matches(options::OPT_fsan_cov_Group)) { - diagnoseUnsupportedOption(A, *DAL, Args); - continue; - } - - if (A->getOption().matches(options::OPT_fsanitize_EQ) && - !Args.hasFlag(options::OPT_fgpu_sanitize, options::OPT_fno_gpu_sanitize, - true)) - continue; - - DAL->append(A); - } - - if (!BoundArch.empty()) { - DAL->eraseArg(options::OPT_mcpu_EQ); - DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_mcpu_EQ), BoundArch); - } - - return DAL; -} - -void AMDGPUOpenMPToolChain::addClangWarningOptions( - ArgStringList &CC1Args) const { - AMDGPUToolChain::addClangWarningOptions(CC1Args); - HostTC.addClangWarningOptions(CC1Args); -} - ToolChain::CXXStdlibType AMDGPUOpenMPToolChain::GetCXXStdlibType(const ArgList &Args) const { return HostTC.GetCXXStdlibType(Args); diff --git a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.h b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.h index d030246d02cbb..3eaf4e5e8af91 100644 --- a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.h +++ b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.h @@ -33,15 +33,10 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUOpenMPToolChain final return &HostTC.getTriple(); } - llvm::opt::DerivedArgList * - TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef BoundArch, - Action::OffloadKind DeviceOffloadKind) const override; - void addClangTargetOptions(const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args, Action::OffloadKind DeviceOffloadKind) const override; - void addClangWarningOptions(llvm::opt::ArgStringList &CC1Args) const override; CXXStdlibType GetCXXStdlibType(const llvm::opt::ArgList &Args) const override; void AddClangCXXStdlibIncludeArgs( const llvm::opt::ArgList &Args, diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp b/clang/lib/Driver/ToolChains/Gnu.cpp index 131dd725c7289..95453fd7076eb 100644 --- a/clang/lib/Driver/ToolChains/Gnu.cpp +++ b/clang/lib/Driver/ToolChains/Gnu.cpp @@ -3458,8 +3458,8 @@ llvm::opt::DerivedArgList * Generic_GCC::TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef BoundArch, Action::OffloadKind DeviceOffloadKind) const { - if (DeviceOffloadKind != Action::OFK_SYCL && - DeviceOffloadKind != Action::OFK_OpenMP) + if (DeviceOffloadKind == Action::OFK_None || + DeviceOffloadKind == Action::OFK_Host) return nullptr; DerivedArgList *DAL = new DerivedArgList(Args.getBaseArgs()); @@ -3485,12 +3485,19 @@ Generic_GCC::TranslateArgs(const llvm::opt::DerivedArgList &Args, // Add the bound architecture to the arguments list if present. if (!BoundArch.empty()) { - options::ID Opt = - getTriple().isARM() || getTriple().isPPC() || getTriple().isAArch64() - ? options::OPT_mcpu_EQ - : options::OPT_march_EQ; - DAL->eraseArg(Opt); - DAL->AddJoinedArg(nullptr, Opts.getOption(Opt), BoundArch); + if (getTriple().isAMDGPU()) { + for (options::ID OptID : {options::OPT_mcpu_EQ, options::OPT_march_EQ}) { + DAL->eraseArg(OptID); + DAL->AddJoinedArg(nullptr, Opts.getOption(OptID), BoundArch); + } + } else { + options::ID Opt = + getTriple().isARM() || getTriple().isPPC() || getTriple().isAArch64() + ? options::OPT_mcpu_EQ + : options::OPT_march_EQ; + DAL->eraseArg(Opt); + DAL->AddJoinedArg(nullptr, Opts.getOption(Opt), BoundArch); + } } return DAL; } diff --git a/clang/lib/Driver/ToolChains/HIPAMD.cpp b/clang/lib/Driver/ToolChains/HIPAMD.cpp index 9211803aa6a2f..1d833076b0a02 100644 --- a/clang/lib/Driver/ToolChains/HIPAMD.cpp +++ b/clang/lib/Driver/ToolChains/HIPAMD.cpp @@ -235,8 +235,6 @@ HIPAMDToolChain::HIPAMDToolChain(const Driver &D, const llvm::Triple &Triple, void HIPAMDToolChain::addClangTargetOptions( const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args, Action::OffloadKind DeviceOffloadingKind) const { - HostTC.addClangTargetOptions(DriverArgs, CC1Args, DeviceOffloadingKind); - assert(DeviceOffloadingKind == Action::OFK_HIP && "Only HIP offloading kinds are supported for GPUs."); @@ -292,37 +290,14 @@ llvm::opt::DerivedArgList * HIPAMDToolChain::TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef BoundArch, Action::OffloadKind DeviceOffloadKind) const { - DerivedArgList *DAL = - HostTC.TranslateArgs(Args, BoundArch, DeviceOffloadKind); - if (!DAL) - DAL = new DerivedArgList(Args.getBaseArgs()); - - const OptTable &Opts = getDriver().getOpts(); - - for (Arg *A : Args) { - // Sanitizer coverage is currently not supported for AMDGPU. - if (A->getOption().matches(options::OPT_fsan_cov_Group)) { - diagnoseUnsupportedOption(A, *DAL, Args); - continue; - } - - if (A->getOption().matches(options::OPT_fsanitize_EQ) && - !Args.hasFlag(options::OPT_fgpu_sanitize, options::OPT_fno_gpu_sanitize, - true)) - continue; - - DAL->append(A); - } + llvm::opt::DerivedArgList *DAL = + ROCMToolChain::TranslateArgs(Args, BoundArch, DeviceOffloadKind); - if (!BoundArch.empty()) { - DAL->eraseArg(options::OPT_mcpu_EQ); - DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_mcpu_EQ), BoundArch); - checkTargetID(*DAL); - } - - if (!Args.hasArg(options::OPT_flto_partitions_EQ)) + if (!Args.hasArg(options::OPT_flto_partitions_EQ)) { + const OptTable &Opts = getDriver().getOpts(); DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_flto_partitions_EQ), "8"); + } return DAL; } @@ -333,11 +308,6 @@ Tool *HIPAMDToolChain::buildLinker() const { return new tools::AMDGCN::Linker(*this); } -void HIPAMDToolChain::addClangWarningOptions(ArgStringList &CC1Args) const { - AMDGPUToolChain::addClangWarningOptions(CC1Args); - HostTC.addClangWarningOptions(CC1Args); -} - ToolChain::CXXStdlibType HIPAMDToolChain::GetCXXStdlibType(const ArgList &Args) const { return HostTC.GetCXXStdlibType(Args); diff --git a/clang/lib/Driver/ToolChains/HIPAMD.h b/clang/lib/Driver/ToolChains/HIPAMD.h index ef1ebb83c1023..fd11da6d9096c 100644 --- a/clang/lib/Driver/ToolChains/HIPAMD.h +++ b/clang/lib/Driver/ToolChains/HIPAMD.h @@ -69,7 +69,6 @@ class LLVM_LIBRARY_VISIBILITY HIPAMDToolChain final : public ROCMToolChain { addClangTargetOptions(const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args, Action::OffloadKind DeviceOffloadKind) const override; - void addClangWarningOptions(llvm::opt::ArgStringList &CC1Args) const override; CXXStdlibType GetCXXStdlibType(const llvm::opt::ArgList &Args) const override; void AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs, diff --git a/clang/test/Driver/offload-darwin-host.hip b/clang/test/Driver/offload-darwin-host.hip new file mode 100644 index 0000000000000..3a3ab7267bd70 --- /dev/null +++ b/clang/test/Driver/offload-darwin-host.hip @@ -0,0 +1,8 @@ +// Make sure the darwin host toolchain does not assert with offload +// languages. The darwin toolchain has a platform initialization hack which was +// broken by offload toolchains trying to perform extra host argument handling. + +// RUN: %clang -### --target=arm64-apple-darwin24.6.0 -mmacos-version-min=10.9 --offload-arch=gfx900 -nogpulib -nogpuinc %s 2>&1 | FileCheck %s + +// CHECK: "-cc1" "-triple" "amdgcn-amd-amdhsa" "-aux-triple" "arm64-apple-darwin24.6.0" +// CHECK: "-cc1" "-triple" "arm64-apple-macosx10.9.0" "-aux-triple" "amdgcn-amd-amdhsa" diff --git a/clang/test/Driver/openmp-Xopenmp-target-forward-args.c b/clang/test/Driver/openmp-Xopenmp-target-forward-args.c new file mode 100644 index 0000000000000..5749df973ed78 --- /dev/null +++ b/clang/test/Driver/openmp-Xopenmp-target-forward-args.c @@ -0,0 +1,10 @@ +// Check that -Xopenmp-target forwards arbitrary arguments, not just -march + +// RUN: %clang -### --target=x86_64-pc-linux -no-canonical-prefixes -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx900 -Xopenmp-target=amdgcn-amd-amdhsa -ffinite-math-only -nogpulib %s 2>&1 \ +// RUN: | FileCheck %s + +// Flag should only apply to device, not the host. + +// CHECK-NOT: -ffinite-math-only +// CHECK: "-triple" "amdgcn-amd-amdhsa" {{.*}} "-ffinite-math-only" +// CHECK-NOT: -ffinite-math-only _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
