[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

6071 ↗

(On Diff #220214)

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

134 ↗

(On Diff #220214)

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 ↗

(On Diff #224854)

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 ↗

(On Diff #224854)

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 ↗

(On Diff #224854)

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/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);
+
+    // Pass the features to linker.
+    std::vector<StringRef> Features;
+    riscv::getRISCVTargetFeatures(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(Args.MakeArgString(Twine("-plugin-opt=--"
+                                                 "mattr=") +
+                                           Name));
+    }
+
+    // Pass the ABIName to linker.
+    StringRef ABIName = tools::riscv::getRISCVABI(Args, ToolChain.getTriple());
+    CmdArgs.push_back(Args.MakeArgString(Twine("-plugin-opt=-target-"
+                                               "abi=") +
+                                         ABIName));
+  }
+
   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
+// CHECK-RISCV: "-plugin-opt=--mattr=+m"
+// CHECK-RISCV: "-plugin-opt=--mattr=+f"
+// CHECK-RISCV: "-plugin-opt=--mattr=+relax"
+// CHECK-RISCV: "-plugin-opt=-target-abi=lp64f"