Diagnose invalid target ID for AMDGPU toolchain for assembler

[yaxunl]

Summary

AMDGPU toolchain currently only diagnose invalid target ID for OpenCL
source compilation. Invalid target ID is not diagnosed for assembler.

This patch fixes that.

[ashi1]

Could you please add full context? Mostly looks good to me.

clang/lib/Driver/ToolChains/HIP.cpp

243

Why move the checking logic to the end if getGPUArch doesn't do any checking?

[yaxunl]

update patch with full context

[yaxunl]

clang/lib/Driver/ToolChains/HIP.cpp

243

It is moved to a different function which is called for both source file compilation and assembler.

[yaxunl]

fix bug

[ashi1]

LGTM

clang/lib/Driver/ToolChains/AMDGPU.h

public:
 
   /// Needed for translating LTO options.
   const char *getDefaultLinker() const override { return "ld.lld"; }
 
   /// Should skip argument.
   bool shouldSkipArgument(const llvm::opt::Arg *Arg) const;
 
 protected:
-  /// Translate -mcpu option containing target ID to cc1 options.
-  /// Returns the GPU name.
-  StringRef translateTargetID(const llvm::opt::ArgList &DriverArgs,
-                              llvm::opt::ArgStringList &CC1Args) const;
+  /// Check and diagnose invalid target ID specified by -mcpu.
+  void checkTargetID(const llvm::opt::ArgList &DriverArgs) const;
 
+  /// Get GPU arch from -mcpu without checking.
   StringRef getGPUArch(const llvm::opt::ArgList &DriverArgs) const;
 };
 
 class LLVM_LIBRARY_VISIBILITY ROCMToolChain : public AMDGPUToolChain {
 public:
   ROCMToolChain(const Driver &D, const llvm::Triple &Triple,
                 const llvm::opt::ArgList &Args);
   void

clang/lib/Driver/ToolChains/AMDGPU.cpp

AMDGPUToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch,
   if (!DAL)
     DAL = new DerivedArgList(Args.getBaseArgs());
 
   for (Arg *A : Args) {
     if (!shouldSkipArgument(A))
       DAL->append(A);
   }
 
+  checkTargetID(*DAL);
+
   if (!Args.getLastArgValue(options::OPT_x).equals("cl"))
     return DAL;
 
   // Phase 1 (.cl -> .bc)
   if (Args.hasArg(options::OPT_c) && Args.hasArg(options::OPT_emit_llvm)) {
     DAL->AddFlagArg(nullptr, Opts.getOption(getTriple().isArch64Bit()
                                                 ? options::OPT_m64
                                                 : options::OPT_m32));
ROCMToolChain::ROCMToolChain(const Driver &D, const llvm::Triple &Triple,
     : AMDGPUToolChain(D, Triple, Args) {
   RocmInstallation.detectDeviceLibrary();
 }
 
 void AMDGPUToolChain::addClangTargetOptions(
     const llvm::opt::ArgList &DriverArgs,
     llvm::opt::ArgStringList &CC1Args,
     Action::OffloadKind DeviceOffloadingKind) const {
-  // Allow using target ID in -mcpu.
-  translateTargetID(DriverArgs, CC1Args);
   // Default to "hidden" visibility, as object level linking will not be
   // supported for the foreseeable future.
   if (!DriverArgs.hasArg(options::OPT_fvisibility_EQ,
                          options::OPT_fvisibility_ms_compat)) {
     CC1Args.push_back("-fvisibility");
     CC1Args.push_back("hidden");
     CC1Args.push_back("-fapply-global-visibility-to-externs");
   }
 }
 
 StringRef
 AMDGPUToolChain::getGPUArch(const llvm::opt::ArgList &DriverArgs) const {
   return getProcessorFromTargetID(
       getTriple(), DriverArgs.getLastArgValue(options::OPT_mcpu_EQ));
 }
 
-StringRef
-AMDGPUToolChain::translateTargetID(const llvm::opt::ArgList &DriverArgs,
-                                   llvm::opt::ArgStringList &CC1Args) const {
+void AMDGPUToolChain::checkTargetID(
+    const llvm::opt::ArgList &DriverArgs) const {
   StringRef TargetID = DriverArgs.getLastArgValue(options::OPT_mcpu_EQ);
   if (TargetID.empty())
-    return StringRef();
+    return;
 
   llvm::StringMap<bool> FeatureMap;
   auto OptionalGpuArch = parseTargetID(getTriple(), TargetID, &FeatureMap);
   if (!OptionalGpuArch) {
     getDriver().Diag(clang::diag::err_drv_bad_target_id) << TargetID;
-    return StringRef();
   }
-
-  return OptionalGpuArch.getValue();
 }
 
 void ROCMToolChain::addClangTargetOptions(
     const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args,
     Action::OffloadKind DeviceOffloadingKind) const {
   AMDGPUToolChain::addClangTargetOptions(DriverArgs, CC1Args,
                                          DeviceOffloadingKind);
 

clang/lib/Driver/ToolChains/HIP.cpp

 }
 
 void HIPToolChain::addClangTargetOptions(
     const llvm::opt::ArgList &DriverArgs,
     llvm::opt::ArgStringList &CC1Args,
     Action::OffloadKind DeviceOffloadingKind) const {
   HostTC.addClangTargetOptions(DriverArgs, CC1Args, DeviceOffloadingKind);
 
-  // Allow using target ID in --offload-arch.
-  StringRef GpuArch = translateTargetID(DriverArgs, CC1Args);
+  StringRef GpuArch = getGPUArch(DriverArgs);
   assert(!GpuArch.empty() && "Must have an explicit GPU arch.");
   (void) GpuArch;
   assert(DeviceOffloadingKind == Action::OFK_HIP &&
          "Only HIP offloading kinds are supported for GPUs.");
   auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch);
   const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind);
 
   CC1Args.push_back("-fcuda-is-device");
HIPToolChain::TranslateArgs(const llvm::opt::DerivedArgList &Args,
   for (Arg *A : Args) {
     if (!shouldSkipArgument(A))
       DAL->append(A);
   }
 
   if (!BoundArch.empty()) {
     DAL->eraseArg(options::OPT_mcpu_EQ);
     DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_mcpu_EQ), BoundArch);
+    checkTargetID(*DAL);
   }
 
   return DAL;
 }
 
 Tool *HIPToolChain::buildLinker() const {
   assert(getTriple().getArch() == llvm::Triple::amdgcn);
   return new tools::AMDGCN::Linker(*this);

clang/test/Driver/amdgpu-invalid-target-id.s

+// REQUIRES: clang-driver
+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+
+// RUN: not %clang -target amdgcn-amd-amdhsa \
+// RUN:   -mcpu=gfx908xnack -nostdlib \
+// RUN:   %s 2>&1 | FileCheck -check-prefix=NOPLUS %s
+
+// NOPLUS: error: Invalid target ID: gfx908xnack
+
+// RUN: not %clang -target amdgcn-amd-amdpal \
+// RUN:   -mcpu=gfx908:xnack+:xnack+ -nostdlib \
+// RUN:   %s 2>&1 | FileCheck -check-prefix=ORDER %s
+
+// ORDER: error: Invalid target ID: gfx908:xnack+:xnack+
+
+// RUN: not %clang -target amdgcn--mesa3d \
+// RUN:   -mcpu=gfx908:unknown+ -nostdlib \
+// RUN:   %s 2>&1 | FileCheck -check-prefix=UNK %s
+
+// UNK: error: Invalid target ID: gfx908:unknown+
+
+// RUN: not %clang -target amdgcn-amd-amdhsa \
+// RUN:   -mcpu=gfx908:sram-ecc+:unknown+ -nostdlib \
+// RUN:   %s 2>&1 | FileCheck -check-prefix=MIXED %s
+
+// MIXED: error: Invalid target ID: gfx908:sram-ecc+:unknown+
+
+// RUN: not %clang -target amdgcn-amd-amdhsa \
+// RUN:   -mcpu=gfx900:sram-ecc+ -nostdlib \
+// RUN:   %s 2>&1 | FileCheck -check-prefix=UNSUP %s
+
+// UNSUP: error: Invalid target ID: gfx900:sram-ecc+
+
+// RUN: not %clang -target amdgcn-amd-amdhsa \
+// RUN:   -mcpu=gfx900:xnack -nostdlib \
+// RUN:   %s 2>&1 | FileCheck -check-prefix=NOSIGN %s
+
+// NOSIGN: error: Invalid target ID: gfx900:xnack
+
+// RUN: not %clang -target amdgcn-amd-amdhsa \
+// RUN:   -mcpu=gfx900+xnack -nostdlib \
+// RUN:   %s 2>&1 | FileCheck -check-prefix=NOCOLON %s
+
+// NOCOLON: error: Invalid target ID: gfx900+xnack