pass -mabi to LTO linker only in RISC-V targets, enable RISC-V LTO

[khchen]

Summary

ABI(-mabi) is not encoded in bitcode function attribute and it is not suitable to modeled on a per-function basis because one compilation unit need to have the same ABI in risc-v .

in this patch driver will pass -mabi to LTO via option, but there is another passing way via module metadata,
I'm not sure which solution is better.

any suggestion?

[khchen]

update missed code..

[lenary]

Nice! I think this is the correct way of implementing this, and I don't think it will have any issue with other backends.

[efriedma]

My primary concern with this is that I'm not sure you should be passing this information separately, as opposed to encoding it into the bitcode.

On ARM, the ABI for a file is generally derived from the target triple. For example, "arm-eabi" is soft-float, "arm-eabihf" is hard-float. This makes the intended target clear, provides some protection against accidentally using LTO with files compiled for different ABIs, and matches up well with our existing configuration flags to set a default target for a compiler.

For other ABI-ish sorts of questions, LLVM IR also supports module flags, which can specify various parameters that apply to an entire object file. See, for example, http://llvm.org/docs/LangRef.html#c-type-width-module-flags-metadata . This has some of the same benefits: it clearly applies to the whole file, and it detects mixing targets with LTO.

[Jim]

This patch is only for RISCV.
The title should have prefix "[RISCV]".

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

727

Argument list should be aligned.

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

31

Argument list should be aligned.

[khchen]

In D71387#1786518, @efriedma wrote:

My primary concern with this is that I'm not sure you should be passing this information separately, as opposed to encoding it into the bitcode.

On ARM, the ABI for a file is generally derived from the target triple. For example, "arm-eabi" is soft-float, "arm-eabihf" is hard-float. This makes the intended target clear, provides some protection against accidentally using LTO with files compiled for different ABIs, and matches up well with our existing configuration flags to set a default target for a compiler.

For other ABI-ish sorts of questions, LLVM IR also supports module flags, which can specify various parameters that apply to an entire object file. See, for example, http://llvm.org/docs/LangRef.html#c-type-width-module-flags-metadata . This has some of the same benefits: it clearly applies to the whole file, and it detects mixing targets with LTO.

@efriedma
Thank you for your information.
Unfortunately on RISCV, the ABI info is only derived from -mabi option and the target triple does not encode ABI info (same to gcc).
In addition, the resulting object file only uses one ABI.

If we choose the module flags metadata solution, there are some problems about which ABI should be chosen when linking different ABIs.

example 1:
clang -target riscv64-unknown-elf a.c -flto -march=rv64imf -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64i -mabi=lp64 -o b.o
clang -target riscv64-unknown-elf -march=rv64imf -mabi=lp64f a.o b.o -flto -o foo

What's expected behavior? user specifics -mabi=lp64f so the result object ABI is lp64f?

example 2:

clang -target riscv64-unknown-elf a.c -flto -march=rv64imf -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64i -mabi=lp64f -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -o foo

What's expected behavior? The result object is -mabi=lp64f because they have same ABI in module metadata?

example 3:

clang -target riscv64-unknown-elf a.c -flto -march=rv64imf -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64imf -mabi=lp64 -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -o foo

What's expected behavior? Use the default march and mabi?

RISCV clang driver has default march and mabi, so I think maybe it's a easier and clear way to overwrite ABI via option only for RISCV target,
and there is no problem (I think) when overwriting the original ABI. (so the mixing ABI is not a problem)

BUT in https://reviews.llvm.org/D67385#1680959 @tejohnson taught me that "LLVM are going more and more toward a model where info is communicated via the IR from the compile step",
So I'm wondering if passing the ABI via option to LTO ONLY for specific target, is it still a bad ideal?

Thanks.

[khchen]

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

727

I will fixed it if when this patch is acceptable, thanks.

[efriedma]

Unfortunately on RISCV, the ABI info is only derived from -mabi option and the target triple does not encode ABI info (same to gcc).

How does gcc decide the default ABI for a target? Do you explicitly specify it at configure time?

example 1:
What's expected behavior? user specifics -mabi=lp64f so the result object ABI is lp64f?

I think the expected behavior is a fatal error. There isn't any reason to let people shoot themselves in the foot like this. (module flags have builtin support for producing such an error.)

example 2:
What's expected behavior? The result object is -mabi=lp64f because they have same ABI in module metadata?

Yes.

there is no problem (I think) when overwriting the original ABI. (so the mixing ABI is not a problem)

clang emits different IR for hardfloat vs. softfloat targets; if you try to overwrite the ABI of a bitcode file, we'll generate wrong code.

[khchen]

In D71387#1788472, @efriedma wrote:

Unfortunately on RISCV, the ABI info is only derived from -mabi option and the target triple does not encode ABI info (same to gcc).

How does gcc decide the default ABI for a target? Do you explicitly specify it at configure time?

Yes, default March/ABI are decided in configure time, and there is also default March/ABI there.

example 1:
What's expected behavior? user specifics -mabi=lp64f so the result object ABI is lp64f?

I think the expected behavior is a fatal error. There isn't any reason to let people shoot themselves in the foot like this. (module flags have builtin support for producing such an error.)

example 2:
What's expected behavior? The result object is -mabi=lp64f because they have same ABI in module metadata?

Yes.

there is no problem (I think) when overwriting the original ABI. (so the mixing ABI is not a problem)

clang emits different IR for hardfloat vs. softfloat targets; if you try to overwrite the ABI of a bitcode file, we'll generate wrong code.

Do you mean different function attributes for different ABI targets?
I see there are arm_aapcs_vfpcc annotation and use-soft-float=false in attributes when giving the -mfloat-abi=hard option.
But in RISCV clang emits the same IR for different ABI (-mabi), so maybe it is no problem in RISCV? (-mfloat-abi is unused in riscv clang driver)

[khchen]

I asked @kito-cheng about the GCC LTO behavior,
GCC LTO encodes the ABI info in elf and the behavior in above examples match to @efriedma 's responses.
So I think maybe encode ABI in module metadata flag is a good ideal.

[efriedma]

But in RISCV clang emits the same IR for different ABI (-mabi),

This is not true. For simple cases, it does, yes, but there are some weird edge cases for functions with many arguments.

[khchen]

In D71387#1790413, @efriedma wrote:

But in RISCV clang emits the same IR for different ABI (-mabi),

This is not true. For simple cases, it does, yes, but there are some weird edge cases for functions with many arguments.

It's interesting, I didn't find the edge cases for functions with many arguments.
but you are right, I found a case when passing struct as argument,
different -mabi will generate incompatible function interface.

struct bar{
int a;
float b;
};

float foo(struct bar a, struct bar b){ ...}

compiled with -march=rv32imafc -mabi=ilp32f: define dso_local float @foo(i32 %0, float %1, i32 %2, float %3)
compiled with -march=rv32imafc -mabi=-ilp32: define dso_local float @foo([2 x i32] %a.coerce, [2 x i32] %b.coerce)

@efriedma thanks for your suggestions.

[khchen]

I'd like to discuss what is good way to pass target-abi in maillist after I had a try to encoding ABI info into LLVM IR via module flag.
any suggestions are welcome, thanks.

[lenary]

@efriedma To keep you in the loop, D72624 is the patch to add a target hook for setting the TargetMachine options based on module metadata. That patch does not add any RISC-V specific functionality, it only lays the groundwork for it.

[efriedma]

Okay. Please let me know if you want me to review anything.

[khchen]

Pass -target-abi option into LTO codegenerator base on D102582 patch.

please see D102582 for more detal.

[jrtc27]

clang/test/Driver/lto.c

81–90

This isn't a great test, anything that unconditionally passes -plugin-opt=target-abi=lp64f passes it. Test RV32 vs RV64 and with multiple ABIs for each to have confidence it does the right thing. Also have a test for not passing anything.

[khchen]

In D71387#1820995, @efriedma wrote:

Okay. Please let me know if you want me to review anything.

Hi all,
We had encoded the target-abi into module now, but I feel it does not make sense to
support overwrite ABI option and datalayout in TargetMahcine/IR by target-abi module flag in IR.

So I think maybe passing the target-abi option by clang driver can make anything more simple, the only one limitation is users need to specific -mabi in below cases at the last command.

clang -target riscv64-unknown-elf a.c -flto -march=rv64gc -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64gc -mabi=lp64f -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -march=rv64gc -o foo

[jrtc27]

In D71387#2762115, @khchen wrote:

In D71387#1820995, @efriedma wrote:

Okay. Please let me know if you want me to review anything.

Hi all,
We had encoded the target-abi into module now, but I feel it does not make sense to
support overwrite ABI option and datalayout in TargetMahcine/IR by target-abi module flag in IR.

So I think maybe passing the target-abi option by clang driver can make anything more simple, the only one limitation is users need to specific -mabi in below cases at the last command.

clang -target riscv64-unknown-elf a.c -flto -march=rv64gc -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64gc -mabi=lp64f -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -march=rv64gc -o foo

We should treat a missing -mabi= as an implicit -mabi=whatever-the-default-is for consistency with non-LTO. So yes, if the default ABI differs from what you've compiled the .o's with, you should have to provide it. This is needed already _anyway_ for multilib toolchains to determine the right library search path, though there are cases currently when you can get away without providing it, at least with Clang.

[khchen]

In D71387#2762120, @jrtc27 wrote:

In D71387#2762115, @khchen wrote:

In D71387#1820995, @efriedma wrote:

Okay. Please let me know if you want me to review anything.

Hi all,
We had encoded the target-abi into module now, but I feel it does not make sense to
support overwrite ABI option and datalayout in TargetMahcine/IR by target-abi module flag in IR.

So I think maybe passing the target-abi option by clang driver can make anything more simple, the only one limitation is users need to specific -mabi in below cases at the last command.

clang -target riscv64-unknown-elf a.c -flto -march=rv64gc -mabi=lp64f -o a.o
clang -target riscv64-unknown-elf b.c -flto -march=rv64gc -mabi=lp64f -o b.o
clang -target riscv64-unknown-elf a.o b.o -flto -march=rv64gc -o foo

We should treat a missing -mabi= as an implicit -mabi=whatever-the-default-is for consistency with non-LTO. So yes, if the default ABI differs from what you've compiled the .o's with, you should have to provide it. This is needed already _anyway_ for multilib toolchains to determine the right library search path, though there are cases currently when you can get away without providing it, at least with Clang.

Hi @jrtc27, do you mean in clang, we need encode an explicitly -target-abi string (computed by RISCVABI::computeTargetABI) rather than an empty string in IR module?

[khchen]

Update test cases.

[jrtc27]

clang/lib/Driver/ToolChains/CommonArgs.cpp

633

634–638

ToolChain.getArch().isRISCV()? No need for a switch until there are other architectures that need code here too.

clang/test/Driver/lto.c

118

Give these meaningful names

126–129

Why this set without ilp32 and lp64d, two of the four most common ABIs?

[lewis-revill]

I believe this patch is still relevant/necessary when using LTO for RISCV, so can I ask if @khchen is able to update it to rebase/address the feedback? If not, are there are any objections to me commandeering this revision to get it landed?

[raj.khem]

In D71387#3755056, @lewis-revill wrote:

I believe this patch is still relevant/necessary when using LTO for RISCV, so can I ask if @khchen is able to update it to rebase/address the feedback? If not, are there are any objections to me commandeering this revision to get it landed?

yes indeed, its still needed. I have disabled LTO for RISCV in Yocto for this reason. I will be happy to test out a rebased patch along.

[khchen]

In D71387#3755056, @lewis-revill wrote:

I believe this patch is still relevant/necessary when using LTO for RISCV, so can I ask if @khchen is able to update it to rebase/address the feedback? If not, are there are any objections to me commandeering this revision to get it landed?

Yes, you could command this revision to enable LTO, thank you!!

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

 namespace riscv {
 void getRISCVTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                             const llvm::opt::ArgList &Args,
                             std::vector<llvm::StringRef> &Features);
 StringRef getRISCVABI(const llvm::opt::ArgList &Args,
                       const llvm::Triple &Triple);
 StringRef getRISCVArch(const llvm::opt::ArgList &Args,
                        const llvm::Triple &Triple);
+
+void addRISCVTargetABIArgs(const ToolChain &ToolChain,
+                           const llvm::opt::ArgList &Args,
+                           llvm::opt::ArgStringList &CmdArgs);
 } // end namespace riscv
 } // namespace tools
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ARCH_RISCV_H

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

else
       return "rv32imafdc";
   } else {
     if (Triple.getOS() == llvm::Triple::UnknownOS)
       return "rv64imac";
     else
       return "rv64imafdc";
   }
 }
+
+void riscv::addRISCVTargetABIArgs(const ToolChain &ToolChain,
+                                  const llvm::opt::ArgList &Args,
+                                  llvm::opt::ArgStringList &CmdArgs) {
+  // We need to pass target-abi option to check it is eqaul to module's
+  // target-abi information.
+  StringRef ABIName = getRISCVABI(Args, ToolChain.getTriple());
+  CmdArgs.push_back(
+      Args.MakeArgString(Twine("-plugin-opt=-target-abi=") + ABIName));
+}

clang/lib/Driver/ToolChains/BareMetal.h

protected:
 
   std::string buildCompilerRTBasename(const llvm::opt::ArgList &Args,
                                       StringRef Component,
                                       FileType Type = ToolChain::FT_Static,
                                       bool AddArch = true) const override;
 
 public:
   bool useIntegratedAs() const override { return true; }
+  bool HasNativeLLVMSupport() const override { return true; }
   bool isCrossCompiling() const override { return true; }
   bool isPICDefault() const override { return false; }
   bool isPIEDefault() const override { return false; }
   bool isPICDefaultForced() const override { return false; }
   bool SupportsProfiling() const override { return false; }
 
   StringRef getOSLibName() const override { return "baremetal"; }
 

clang/lib/Driver/ToolChains/BareMetal.cpp

if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
     CmdArgs.push_back("-lm");
 
     TC.AddLinkRuntimeLib(Args, CmdArgs);
   }
 
   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
 
+  const Driver &D = TC.getDriver();
+  if (D.isUsingLTO()) {
+    assert(!Inputs.empty() && "Must have at least one input.");
+    addLTOOptions(TC, Args, CmdArgs, Output, Inputs[0],
+                  D.getLTOMode() == LTOK_Thin);
+  }
+
   C.addCommand(std::make_unique<Command>(JA, *this, ResponseFileSupport::None(),
                                          Args.MakeArgString(TC.GetLinkerPath()),
                                          CmdArgs, Inputs, Output));
 }

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/M68k.h"
 #include "Arch/Mips.h"
 #include "Arch/PPC.h"
+#include "Arch/RISCV.h"
 #include "Arch/SystemZ.h"
 #include "Arch/VE.h"
 #include "Arch/X86.h"
 #include "HIP.h"
 #include "Hexagon.h"
 #include "InputInfo.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/LangOptions.h"
#endif
   // Setup statistics file output.
   SmallString<128> StatsFile = getStatsFileName(Args, Output, Input, D);
   if (!StatsFile.empty())
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-plugin-opt=stats-file=") + StatsFile));
 
   addX86AlignBranchArgs(D, Args, CmdArgs, /*IsLTO=*/true);
 
+  // pass more options in specific target
+  switch (ToolChain.getArch()) {
+  default:
+    break;
+  case llvm::Triple::riscv32:
+  case llvm::Triple::riscv64: {
+    riscv::addRISCVTargetABIArgs(ToolChain, Args, CmdArgs);
+    break;
+  }
+  }
+
   // Handle remark diagnostics on screen options: '-Rpass-*'.
   renderRpassOptions(Args, CmdArgs);
 
   // Handle serialized remarks options: '-fsave-optimization-record'
   // and '-foptimization-record-*'.
   if (willEmitRemarks(Args))
     renderRemarksOptions(Args, CmdArgs, ToolChain.getEffectiveTriple(), Input,
                          Output);

clang/lib/Driver/ToolChains/RISCVToolchain.h

public:
   static bool hasGCCToolchain(const Driver &D, 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;
   RuntimeLibType GetDefaultRuntimeLibType() const override;
   UnwindLibType
   GetUnwindLibType(const llvm::opt::ArgList &Args) 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,
   bool IsRV64 = ToolChain.getArch() == llvm::Triple::riscv64;
   CmdArgs.push_back("-m");
   if (IsRV64) {
     CmdArgs.push_back("elf64lriscv");
   } else {
     CmdArgs.push_back("elf32lriscv");
   }
 
+  if (D.isUsingLTO()) {
+    assert(!Inputs.empty() && "Must have at least one input.");
+    addLTOOptions(ToolChain, Args, CmdArgs, Output, Inputs[0],
+                  D.getLTOMode() == LTOK_Thin);
+  }
+
   std::string Linker = getToolChain().GetLinkerPath();
 
   bool WantCRTs =
       !Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles);
 
   const char *crtbegin, *crtend;
   auto RuntimeLib = ToolChain.GetRuntimeLibType(Args);
   if (RuntimeLib == ToolChain::RLT_Libgcc) {

clang/test/Driver/lto.c

 // -flto passes along an explicit debugger tuning argument.
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto -glldb 2> %t
 // RUN: FileCheck -check-prefix=CHECK-TUNING-LLDB < %t %s
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto -g 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-TUNING < %t %s
 //
 // CHECK-TUNING-LLDB:   "-plugin-opt=-debugger-tune=lldb"
 // CHECK-NO-TUNING-NOT: "-plugin-opt=-debugger-tune
+
+// Need to pass -target-abi in RISC-V target.
+// RUN: %clang -target riscv64-unknown-elf %s -fuse-ld=gold -flto \
+// RUN:   -mabi=lp64f -### 2>&1 | FileCheck %s --check-prefix=RISCV
+// RUN: %clang -target riscv64-unknown-elf %s -fuse-ld=lld -flto \
+// RUN:   -mabi=lp64f -### 2>&1 | FileCheck %s --check-prefix=RISCV
+// RUN: %clang -target riscv64-unknown-linux-gnu %s -fuse-ld=gold -flto \
+// RUN:   -mabi=lp64f -### 2>&1 | FileCheck %s --check-prefix=RISCV
+// RUN: %clang -target riscv64-unknown-linux-gnu %s -fuse-ld=lld -flto \
+// RUN:   -mabi=lp64f -### 2>&1 | FileCheck %s --check-prefix=RISCV
+// RISCV: "-plugin-opt=target-abi=lp64f"