[RISCV] enable LTO support, pass some options to linker.

[khchen]

Summary

1. enable LTO need to pass target feature and abi to LTO code generation RISCV backend need the target feature to decide which extension used in code generation.
2. move getTargetFeatures to CommonArgs.h and add ForLTOPlugin flag
3. add general tools::getTargetABI in CommonArgs.h because different target uses different way to get the target ABI.

[lenary]

Can you add a test for riscv64-unknown-linux-gnu? I think RISCVToolchain.cpp is only used by the riscv64-unknown-elf target, but I could be wrong.

[khchen]

@lenary Yes, you are right. add LTO support for Linux platform.

[MaskRay]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

387 ↗

(On Diff #219941)

gold doesn't support RISC-V, does it?

[kito-cheng]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

387 ↗

(On Diff #219941)

Gold doesn't support RISC-V , but ld.bfd supported same plugin API, so this made clang can run LTO with ld.bfd.

[khchen]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

387 ↗

(On Diff #219941)

I reference AddGoldPlugin to naming this function, is it confusing?

[MaskRay]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

414 ↗

(On Diff #219941)

The single-dash form of mattr: -plugin-opt=-mattr= should be preferred. --mattr is rare.

clang/lib/Driver/ToolChains/Arch/RISCV.h

26 ↗

(On Diff #219941)

The description should probably mention why clang::driver:tools::AddGoldPlugin is insufficient.

[khchen]

This commit is inspired by @MaskRay's suggestion, I think maybe fix the insufficient of clang::driver:tools::AddGoldPlugin is good choose.

[lenary]

I have some nits about explicit comments for arguments and default argument values for backwards compatibility. Other than that, it looks like a nice code cleanup.

clang/lib/Driver/ToolChains/Clang.cpp

6110

This needs /*ForAS=*/true, /*ForLTOPlugin=*/false) so it's clear what the booleans are for. Same for other calls to getTargetFeatures.

clang/lib/Driver/ToolChains/CommonArgs.h

127

The final argument here should probably default to false, so that out-of-tree backends do not break immediately when this patch lands.

[khchen]

Thanks @lenary, I fixed it.

[khchen]

ping

[lenary]

Nice, LGTM

[khchen]

rebase master,
https://reviews.llvm.org/D66003 changed the riscv::getRISCVTargetFeatures interface.

[lenary]

@khchen do you need me to commit this for you?

[khchen]

In D67409#1707940, @lenary wrote:

@khchen do you need me to commit this for you?

Yes, please help me, thanks.

[lenary]

@khchen Thanks for your patch! It is now landed.

[efriedma]

clang/lib/Driver/ToolChains/CommonArgs.cpp

498

I don't think this change is right. In general, target features should be encoded in bitcode files. This allows compiling different files with different target features, and using runtime detection to only run certain codepaths. And it makes sure that we end up with a sane result if the user doesn't pass target feature flags on the link line.

Also, it probably isn't appropriate to make target-independent changes in a commit tagged [RISCV]; most people would assume a change marked like that doesn't have target-independent effects.

(Sorry about the delayed response to this review; I only just ran into this.)

[khchen]

clang/lib/Driver/ToolChains/CommonArgs.cpp

498

I don't think this change is right. In general, target features should be encoded in bitcode files. This allows compiling different files with different target features, and using runtime detection to only run certain codepaths. And it makes sure that we end up with a sane result if the user doesn't pass target feature flags on the link line.

I'm curious about your scenario, LTO will link two bitcodes file into one, so which target-features should be kept in final bitcode since different files have different target features? For example, one target features" is "+armv7-a" and another is "+armv8-a".

I guess maybe your case is they are same target-features in different files, but this patch will overwrite the encoded target-feature as default.

anyway, I agree with you. I found the target features does not encoded in bitcode files when enabling LTO in RISCV, I will fixed it and revert the target feature part, thanks.

Also, it probably isn't appropriate to make target-independent changes in a commit tagged [RISCV]; most people would assume a change marked like that doesn't have target-independent effects.

sorry, I will take care of it in next time.

(Sorry about the delayed response to this review; I only just ran into this.)

[efriedma]

clang/lib/Driver/ToolChains/CommonArgs.cpp

498

Target features are encoded into IR on a per-function basis, so we can keep both.

But yes, I'm more worried about the implicit expectation that the target flags are specified on the link line; this isn't reliably true with common build systems.

[hans]

I've reverted in b1ac1f00716 until it can be fixed properly.

We noticed this in Chromium where we started seeing build spam like:

'+soft-float-abi' is not a recognized feature for this target (ignoring feature)
'+soft-float-abi' is not a recognized feature for this target (ignoring feature)

See https://bugs.chromium.org/p/chromium/issues/detail?id=1023280 for details.

Sorry for not reporting it earlier, but because it didn't break the build per se, we didn't notice until now.

[lenary]

The follow-up patch, implementing this correctly, is D70116.

clang/lib/Driver/ToolChains/Clang.cpp

if (Value == "none") {
     if (Value.getAsInteger(10, Width)) {
       D.Diag(diag::err_drv_invalid_value) << A->getOption().getName() << Value;
       return;
     }
     CmdArgs.push_back(Args.MakeArgString("-mprefer-vector-width=" + Value));
   }
 }
 
-static void getWebAssemblyTargetFeatures(const ArgList &Args,
-                                         std::vector<StringRef> &Features) {
-  handleTargetFeaturesGroup(Args, Features, options::OPT_m_wasm_Features_Group);
-}
-
-static void getTargetFeatures(const ToolChain &TC, const llvm::Triple &Triple,
-                              const ArgList &Args, ArgStringList &CmdArgs,
-                              bool ForAS) {
-  const Driver &D = TC.getDriver();
-  std::vector<StringRef> Features;
-  switch (Triple.getArch()) {
-  default:
-    break;
-  case llvm::Triple::mips:
-  case llvm::Triple::mipsel:
-  case llvm::Triple::mips64:
-  case llvm::Triple::mips64el:
-    mips::getMIPSTargetFeatures(D, Triple, Args, Features);
-    break;
-
-  case llvm::Triple::arm:
-  case llvm::Triple::armeb:
-  case llvm::Triple::thumb:
-  case llvm::Triple::thumbeb:
-    arm::getARMTargetFeatures(TC, Triple, Args, CmdArgs, Features, ForAS);
-    break;
-
-  case llvm::Triple::ppc:
-  case llvm::Triple::ppc64:
-  case llvm::Triple::ppc64le:
-    ppc::getPPCTargetFeatures(D, Triple, Args, Features);
-    break;
-  case llvm::Triple::riscv32:
-  case llvm::Triple::riscv64:
-    riscv::getRISCVTargetFeatures(D, Triple, Args, Features);
-    break;
-  case llvm::Triple::systemz:
-    systemz::getSystemZTargetFeatures(Args, Features);
-    break;
-  case llvm::Triple::aarch64:
-  case llvm::Triple::aarch64_be:
-    aarch64::getAArch64TargetFeatures(D, Triple, Args, Features);
-    break;
-  case llvm::Triple::x86:
-  case llvm::Triple::x86_64:
-    x86::getX86TargetFeatures(D, Triple, Args, Features);
-    break;
-  case llvm::Triple::hexagon:
-    hexagon::getHexagonTargetFeatures(D, Args, Features);
-    break;
-  case llvm::Triple::wasm32:
-  case llvm::Triple::wasm64:
-    getWebAssemblyTargetFeatures(Args, Features);
-    break;
-  case llvm::Triple::sparc:
-  case llvm::Triple::sparcel:
-  case llvm::Triple::sparcv9:
-    sparc::getSparcTargetFeatures(D, Args, Features);
-    break;
-  case llvm::Triple::r600:
-  case llvm::Triple::amdgcn:
-    amdgpu::getAMDGPUTargetFeatures(D, Args, Features);
-    break;
-  case llvm::Triple::msp430:
-    msp430::getMSP430TargetFeatures(D, Args, Features);
-  }
-
-  // Find the last of each feature.
-  llvm::StringMap<unsigned> LastOpt;
-  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
-    StringRef Name = Features[I];
-    assert(Name[0] == '-' || Name[0] == '+');
-    LastOpt[Name.drop_front(1)] = I;
-  }
-
-  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
-    // If this feature was overridden, ignore it.
-    StringRef Name = Features[I];
-    llvm::StringMap<unsigned>::iterator LastI = LastOpt.find(Name.drop_front(1));
-    assert(LastI != LastOpt.end());
-    unsigned Last = LastI->second;
-    if (Last != I)
-      continue;
-
-    CmdArgs.push_back("-target-feature");
-    CmdArgs.push_back(Name.data());
-  }
-}
-
 static bool
 shouldUseExceptionTablesForObjCExceptions(const ObjCRuntime &runtime,
                                           const llvm::Triple &Triple) {
   // We use the zero-cost exception tables for Objective-C if the non-fragile
   // ABI is enabled or when compiling for x86_64 and ARM on Snow Leopard and
   // later.
   if (runtime.isNonFragile())
     return true;
void ClangAs::ConstructJob(Compilation &C, const JobAction &JA,
   // Add the target cpu
   std::string CPU = getCPUName(Args, Triple, /*FromAs*/ true);
   if (!CPU.empty()) {
     CmdArgs.push_back("-target-cpu");
     CmdArgs.push_back(Args.MakeArgString(CPU));
   }
 
   // Add the target features
   getTargetFeatures(getToolChain(), Triple, Args, CmdArgs, true);
 
   // Ignore explicit -force_cpusubtype_ALL option.
   (void)Args.hasArg(options::OPT_force__cpusubtype__ALL);
 
   // Pass along any -I options so we get proper .include search paths.
   Args.AddAllArgs(CmdArgs, options::OPT_I_Group);
 
   // Determine the original source input.

clang/lib/Driver/ToolChains/CommonArgs.h

SmallString<128> getStatsFileName(const llvm::opt::ArgList &Args,
                                   const InputInfo &Output,
                                   const InputInfo &Input, const Driver &D);
 
 /// \p Flag must be a flag accepted by the driver with its leading '-' removed,
 //     otherwise '-print-multi-lib' will not emit them correctly.
 void addMultilibFlag(bool Enabled, const char *const Flag,
                      Multilib::flags_list &Flags);
 
+StringRef getTargetABI(const llvm::opt::ArgList &Args,
+                       const llvm::Triple &Triple);
+
+void getTargetFeatures(const ToolChain &TC, const llvm::Triple &Triple,
+                       const llvm::opt::ArgList &Args,
+                       llvm::opt::ArgStringList &CmdArgs, bool ForAS,
+                       bool ForLTOPlugin = false);
+
 } // end namespace tools
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_COMMONARGS_H

clang/lib/Driver/ToolChains/CommonArgs.cpp

 //===--- CommonArgs.cpp - Args handling for multiple toolchains -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CommonArgs.h"
 #include "Arch/AArch64.h"
 #include "Arch/ARM.h"
 #include "Arch/Mips.h"
 #include "Arch/PPC.h"
+#include "Arch/RISCV.h"
+#include "Arch/Sparc.h"
 #include "Arch/SystemZ.h"
 #include "Arch/X86.h"
+#include "AMDGPU.h"
+#include "MSP430.h"
 #include "HIP.h"
 #include "Hexagon.h"
 #include "InputInfo.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/Version.h"
 #include "clang/Config/config.h"
#endif
   }
 
   // Setup statistics file output.
   SmallString<128> StatsFile =
       getStatsFileName(Args, Output, Input, ToolChain.getDriver());
   if (!StatsFile.empty())
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-plugin-opt=stats-file=") + StatsFile));
+
+  getTargetFeatures(ToolChain, ToolChain.getTriple(), Args, CmdArgs,
+                    /* ForAS= */ false, /* ForLTOPlugin= */ true);
+
+  StringRef ABIName = tools::getTargetABI(Args, ToolChain.getTriple());
+  if (!ABIName.empty())
+    CmdArgs.push_back(
+        Args.MakeArgString(Twine("-plugin-opt=-target-abi=") + ABIName));
 }
 
 void tools::addArchSpecificRPath(const ToolChain &TC, const ArgList &Args,
                                  ArgStringList &CmdArgs) {
   if (!Args.hasFlag(options::OPT_frtlib_add_rpath,
                     options::OPT_fno_rtlib_add_rpath, false))
     return;
 
SmallString<128> tools::getStatsFileName(const llvm::opt::ArgList &Args,
   llvm::sys::path::replace_extension(StatsFile, "stats");
   return StatsFile;
 }
 
 void tools::addMultilibFlag(bool Enabled, const char *const Flag,
                             Multilib::flags_list &Flags) {
   Flags.push_back(std::string(Enabled ? "+" : "-") + Flag);
 }
+
+static void getWebAssemblyTargetFeatures(const ArgList &Args,
+                                         std::vector<StringRef> &Features) {
+  handleTargetFeaturesGroup(Args, Features, options::OPT_m_wasm_Features_Group);
+}
+
+void tools::getTargetFeatures(const ToolChain &TC, const llvm::Triple &Triple,
+                       const ArgList &Args, ArgStringList &CmdArgs, bool ForAS,
+                       bool ForLTOPlugin) {
+
+  const Driver &D = TC.getDriver();
+  std::vector<StringRef> Features;
+  switch (Triple.getArch()) {
+  default:
+    break;
+  case llvm::Triple::mips:
+  case llvm::Triple::mipsel:
+  case llvm::Triple::mips64:
+  case llvm::Triple::mips64el:
+    mips::getMIPSTargetFeatures(D, Triple, Args, Features);
+    break;
+
+  case llvm::Triple::arm:
+  case llvm::Triple::armeb:
+  case llvm::Triple::thumb:
+  case llvm::Triple::thumbeb:
+    arm::getARMTargetFeatures(TC, Triple, Args, CmdArgs, Features, ForAS);
+    break;
+
+  case llvm::Triple::ppc:
+  case llvm::Triple::ppc64:
+  case llvm::Triple::ppc64le:
+    ppc::getPPCTargetFeatures(D, Triple, Args, Features);
+    break;
+  case llvm::Triple::riscv32:
+  case llvm::Triple::riscv64:
+    riscv::getRISCVTargetFeatures(D, Triple, Args, Features);
+    break;
+  case llvm::Triple::systemz:
+    systemz::getSystemZTargetFeatures(Args, Features);
+    break;
+  case llvm::Triple::aarch64:
+  case llvm::Triple::aarch64_be:
+    aarch64::getAArch64TargetFeatures(D, Triple, Args, Features);
+    break;
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+    x86::getX86TargetFeatures(D, Triple, Args, Features);
+    break;
+  case llvm::Triple::hexagon:
+    hexagon::getHexagonTargetFeatures(D, Args, Features);
+    break;
+  case llvm::Triple::wasm32:
+  case llvm::Triple::wasm64:
+    getWebAssemblyTargetFeatures(Args, Features);
+    break;
+  case llvm::Triple::sparc:
+  case llvm::Triple::sparcel:
+  case llvm::Triple::sparcv9:
+    sparc::getSparcTargetFeatures(D, Args, Features);
+    break;
+  case llvm::Triple::r600:
+  case llvm::Triple::amdgcn:
+    amdgpu::getAMDGPUTargetFeatures(D, Args, Features);
+    break;
+  case llvm::Triple::msp430:
+    msp430::getMSP430TargetFeatures(D, Args, Features);
+  }
+
+  // Find the last of each feature.
+  llvm::StringMap<unsigned> LastOpt;
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+    StringRef Name = Features[I];
+    assert(Name[0] == '-' || Name[0] == '+');
+    LastOpt[Name.drop_front(1)] = I;
+  }
+
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+    // If this feature was overridden, ignore it.
+    StringRef Name = Features[I];
+    llvm::StringMap<unsigned>::iterator LastI =
+        LastOpt.find(Name.drop_front(1));
+    assert(LastI != LastOpt.end());
+    unsigned Last = LastI->second;
+    if (Last != I)
+      continue;
+    if (!ForLTOPlugin) {
+      CmdArgs.push_back("-target-feature");
+      CmdArgs.push_back(Name.data());
+    } else {
+      CmdArgs.push_back(
+          Args.MakeArgString(Twine("-plugin-opt=-mattr=") + Name));
+    }
+  }
+}
+
+StringRef tools::getTargetABI(const ArgList &Args, const llvm::Triple &Triple) {
+  // TODO: Support the other target ABI
+  switch (Triple.getArch()) {
+  default:
+    break;
+  case llvm::Triple::riscv32:
+  case llvm::Triple::riscv64:
+    return tools::riscv::getRISCVABI(Args, Triple);
+    break;
+  }
+  return StringRef();
+}

clang/lib/Driver/ToolChains/RISCVToolchain.h

 public:
   RISCVToolChain(const Driver &D, const llvm::Triple &Triple,
                  const llvm::opt::ArgList &Args);
 
   bool IsIntegratedAssemblerDefault() const override { return true; }
   void addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
                              llvm::opt::ArgStringList &CC1Args,
                              Action::OffloadKind) const override;
+  bool HasNativeLLVMSupport() const override { return true; }
   void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                             llvm::opt::ArgStringList &CC1Args) const override;
   void
   addLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                            llvm::opt::ArgStringList &CC1Args) const override;
 
 protected:

clang/lib/Driver/ToolChains/RISCVToolchain.cpp

 //===--- RISCVToolchain.cpp - RISCV ToolChain Implementations ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "RISCVToolchain.h"
+#include "Arch/RISCV.h"
 #include "CommonArgs.h"
 #include "InputInfo.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Options.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
void RISCV::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   const Driver &D = ToolChain.getDriver();
   ArgStringList CmdArgs;
 
   if (!D.SysRoot.empty())
     CmdArgs.push_back(Args.MakeArgString("--sysroot=" + D.SysRoot));
 
   std::string Linker = getToolChain().GetProgramPath(getShortName());
 
+  if (D.isUsingLTO()) {
+    assert(!Inputs.empty() && "Must have at least one input.");
+    AddGoldPlugin(ToolChain, Args, CmdArgs, Output, Inputs[0],
+                  D.getLTOMode() == LTOK_Thin);
+  }
+
   bool WantCRTs =
       !Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles);
 
   if (WantCRTs) {
     CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crt0.o")));
     CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crtbegin.o")));
   }
 

clang/test/Driver/gold-lto.c

 // CHECK-ARM-V7A: "-plugin" "{{.*}}{{[/\\]}}LLVMgold.{{dll|dylib|so}}"
 // CHECK-ARM-V7A: "-plugin-opt=mcpu=generic"
 // CHECK-ARM-V7A: "-plugin-opt=O0"
 // CHECK-ARM-V7A: "-plugin-opt=foo"
 //
 // RUN: %clang -target i686-linux-android -### %t.o -flto 2>&1 \
 // RUN:     | FileCheck %s --check-prefix=CHECK-X86-ANDROID
 // CHECK-X86-ANDROID: "-plugin" "{{.*}}{{[/\\]}}LLVMgold.{{dll|dylib|so}}"
+//
+// RUN: %clang -target riscv64-unknown-elf -### %t.o -flto 2>&1 \
+// RUN:     -march=rv64imf -mabi=lp64f \
+// RUN:     | FileCheck %s --check-prefix=CHECK-RISCV-BAREMETAL
+// CHECK-RISCV-BAREMETAL: "-plugin" "{{.*}}{{[/\\]}}LLVMgold.{{dll|dylib|so}}"
+// CHECK-RISCV-BAREMETAL: "-plugin-opt=-mattr=+m"
+// CHECK-RISCV-BAREMETAL: "-plugin-opt=-mattr=+f"
+// CHECK-RISCV-BAREMETAL: "-plugin-opt=-mattr=+relax"
+// CHECK-RISCV-BAREMETAL: "-plugin-opt=-target-abi=lp64f"
+//
+// RUN: %clang -target riscv64-unknown-linux-gnu -### %t.o -flto 2>&1 \
+// RUN:     -march=rv64imf -mabi=lp64f \
+// RUN:     | FileCheck %s --check-prefix=CHECK-RISCV-LINUX
+// CHECK-RISCV-LINUX: "-plugin" "{{.*}}{{[/\\]}}LLVMgold.{{dll|dylib|so}}"
+// CHECK-RISCV-LINUX: "-plugin-opt=-mattr=+m"
+// CHECK-RISCV-LINUX: "-plugin-opt=-mattr=+f"
+// CHECK-RISCV-LINUX: "-plugin-opt=-mattr=+relax"
+// CHECK-RISCV-LINUX: "-plugin-opt=-target-abi=lp64f"