[Clang] Adjust PIC handling for the AMDGPU ToolChain

[jhuber6]

Summary

The AMDGPU target only emits shared libraries currently. This patch
changes the handling of the PIC level to be managed in the
AMDGPUToolChain rather than having a special case for it. This causes
--target=amdgcn-- to no longer set the PIC. This should be an
acceptable change since that doesn't use a correct toolchain anyway.

[arsenm]

I don't understand how the PIC options are supposed to propagate given that codegen seems to still use magic flags but removing a special case seems good

clang/lib/Driver/ToolChains/AMDGPU.h

71

What's the difference between isPICDefault and isPICDefaultForced?

[jhuber6]

clang/lib/Driver/ToolChains/AMDGPU.h

71

isPICDefaultForced more or less just overrides -fpie or similar AFAIK.

clang/lib/Driver/ToolChains/AMDGPU.h

 
 public:
   AMDGPUToolChain(const Driver &D, const llvm::Triple &Triple,
                   const llvm::opt::ArgList &Args);
   unsigned GetDefaultDwarfVersion() const override { return 5; }
 
   bool IsMathErrnoDefault() const override { return false; }
   bool isCrossCompiling() const override { return true; }
-  bool isPICDefault() const override { return false; }
+  bool isPICDefault() const override { return true; }
   bool isPIEDefault(const llvm::opt::ArgList &Args) const override {
     return false;
   }
-  bool isPICDefaultForced() const override { return false; }
+  bool isPICDefaultForced() const override { return true; }
   bool SupportsProfiling() const override { return false; }
 
   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,
public:
   /// Needed for using lto.
   bool HasNativeLLVMSupport() const override {
     return true;
   }
 
   /// Needed for translating LTO options.
   const char *getDefaultLinker() const override { return "ld.lld"; }
 
-  /// Should skip argument.
-  bool shouldSkipArgument(const llvm::opt::Arg *Arg) const;
-
   /// Uses amdgpu-arch tool to get arch of the system GPU. Will return error
   /// if unable to find one.
   virtual Expected<SmallVector<std::string>>
   getSystemGPUArchs(const llvm::opt::ArgList &Args) const override;
 
 protected:
   /// Check and diagnose invalid target ID specified by -mcpu.
   virtual void checkTargetID(const llvm::opt::ArgList &DriverArgs) const;

clang/lib/Driver/ToolChains/AMDGPU.cpp

AMDGPUToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch,
   DerivedArgList *DAL =
       Generic_ELF::TranslateArgs(Args, BoundArch, DeviceOffloadKind);
 
   const OptTable &Opts = getDriver().getOpts();
 
   if (!DAL)
     DAL = new DerivedArgList(Args.getBaseArgs());
 
-  for (Arg *A : Args) {
-    if (!shouldSkipArgument(A))
-      DAL->append(A);
-  }
+  for (Arg *A : Args)
 
   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)) {
RocmInstallationDetector::getCommonBitcodeLibs(
   AddBCLib(LibDeviceFile);
   auto ABIVerPath = getABIVersionPath(ABIVer);
   if (!ABIVerPath.empty())
     AddBCLib(ABIVerPath);
 
   return BCLibs;
 }
 
-bool AMDGPUToolChain::shouldSkipArgument(const llvm::opt::Arg *A) const {
-  Option O = A->getOption();
-  if (O.matches(options::OPT_fPIE) || O.matches(options::OPT_fpie))
-    return true;
-  return false;
-}
-
 llvm::SmallVector<std::string, 12>
 ROCMToolChain::getCommonDeviceLibNames(const llvm::opt::ArgList &DriverArgs,
                                        const std::string &GPUArch,
                                        bool isOpenMP) const {
   auto Kind = llvm::AMDGPU::parseArchAMDGCN(GPUArch);
   const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind);
 
   StringRef LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);

clang/lib/Driver/ToolChains/CommonArgs.cpp

case llvm::Triple::sparcv9:
       IsPICLevelTwo = true; // "-fPIE"
       break;
 
     default:
       break;
     }
   }
 
-  // AMDGPU-specific defaults for PIC.
-  if (Triple.getArch() == llvm::Triple::amdgcn)
-    PIC = true;
-
   // The last argument relating to either PIC or PIE wins, and no
   // other argument is used. If the last argument is any flavor of the
   // '-fno-...' arguments, both PIC and PIE are disabled. Any PIE
   // option implicitly enables PIC at the same level.
   Arg *LastPICArg = Args.getLastArg(options::OPT_fPIC, options::OPT_fno_PIC,
                                     options::OPT_fpic, options::OPT_fno_pic,
                                     options::OPT_fPIE, options::OPT_fno_PIE,
                                     options::OPT_fpie, options::OPT_fno_pie);

clang/lib/Driver/ToolChains/HIPAMD.cpp

HIPAMDToolChain::TranslateArgs(const llvm::opt::DerivedArgList &Args,
   DerivedArgList *DAL =
       HostTC.TranslateArgs(Args, BoundArch, DeviceOffloadKind);
   if (!DAL)
     DAL = new DerivedArgList(Args.getBaseArgs());
 
   const OptTable &Opts = getDriver().getOpts();
 
   for (Arg *A : Args) {
-    if (!shouldSkipArgument(A) &&
-        !shouldSkipSanitizeOption(*this, Args, BoundArch, A))
+    if (!shouldSkipSanitizeOption(*this, Args, BoundArch, 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);
   }

clang/test/Driver/amdgcn-toolchain-pic.cl

-// RUN: %clang -### --target=amdgcn-- -mcpu=gfx803 %s 2>&1 | FileCheck %s
-// RUN: %clang -### --target=amdgcn-amd- -mcpu=gfx803 %s 2>&1 | FileCheck %s
 // RUN: %clang -### --target=amdgcn-amd-amdhsa -mcpu=gfx803 %s 2>&1 | FileCheck %s
 // RUN: %clang -### --target=amdgcn-amd-amdpal -mcpu=gfx803 %s 2>&1 | FileCheck %s
 // RUN: %clang -### --target=amdgcn-amd-mesa3d -mcpu=gfx803 %s 2>&1 | FileCheck %s
 
-// CHECK: "-cc1"{{.*}} "-mrelocation-model" "pic" "-pic-level" "1"
+// CHECK: "-cc1"{{.*}} "-mrelocation-model" "pic" "-pic-level" "2"