[RISCV] Add -mrelax/-mno-relax flags to enable/disable RISCV linker relaxation

[shiva0217]

Summary

Ad -mrelax, -mno-relax flags to enable/disable RISCV linker relaxation

[apazos]

include/clang/Driver/Options.td

1874 ↗

(On Diff #139757)

I think we should define both -mrelax and -mno-relax flags

lib/Driver/ToolChains/Arch/RISCV.cpp

55 ↗

(On Diff #139757)

If we add both mrelax and mno-relax flags, we need to update this logic...

// -mrelax is default, unless -mno-relax is specified.
bool Relax = true;
if (auto *A = Args.getLastArg(options::OPT_mrelax, options::OPT_mno_relax))

if(A->getOption().matches(options::OPT_mno_relax))
  Relax = false;

if (Relax)
Features.push_back("+relax");

[shiva0217]

Update patch to address Ana's comments.

[asb]

Could you please add a test? Given that the current version of D44886 enables linker relaxation by default in the backend, shouldn't -mno-relax cause -relax to be set?

[asb]

In D44888#1058257, @asb wrote:

Could you please add a test? Given that the current version of D44886 enables linker relaxation by default in the backend, shouldn't -mno-relax cause -relax to be set?

Sorry, I misread D444886. Still, we should do Features.push_back("-relax") at least in the case where -mno-relax is given explicitly, meaning we don't have to rely on the backend default.

[shiva0217]

Update patch to address Alex's comments.

[asb]

Looks good to me, just missing help text on the command line options.

[shiva0217]

Add help text for -mrelax, -mno-relax flags as Alex's comments.

[asb]

Thanks Kito. -mrelax and -mno-relax currently only affect the backend. For completeness, I think this patch needs to pass the appropriate flag to the linker depending on relax/no-relax.

[shiva0217]

In D44888#1068806, @asb wrote:

Thanks Kito. -mrelax and -mno-relax currently only affect the backend. For completeness, I think this patch needs to pass the appropriate flag to the linker depending on relax/no-relax.

Hi Alex. RISCVToolChain class does not define yet, so we can't pass --no-relax in RISCV::Linker::ConstructJob. Should we passing --no-relax when we introduce RISCVToolchain? Or there is another place we could implement passing RISCV linker flags?

[kito-cheng]

lib/Driver/ToolChains/Arch/RISCV.cpp

130 ↗

(On Diff #142338)

This part should move to the begin of the function, otherwise it never executed if Exts is empty string.

[shiva0217]

Rebase patch to D45465

[shiva0217]

lib/Driver/ToolChains/Arch/RISCV.cpp

130 ↗

(On Diff #142338)

Hi Kito. I have rebased the patch to D45465. Could you help me to check when you download raw diff and apply with patch -p0< does it still patch to the wrong function?

[asb]

I wonder if it would be safer to change this patch so it adds -mrelax and -mno-relax but doesn't compile with linker relaxation by default. That makes it easier to test linker relaxation support, and gives more time for testing before then flipping to -mrelax as the default.

test/Driver/riscv-features.c

8 ↗

(On Diff #146737)

We need a another RUN line and CHECK here to determine the default whether +relax is passed or not when the user specifies neither -mrelax or -mno-relax.

[asb]

This is looking good to me, just needs an update to address this request for a test in riscv-features.c that demonstrates the default +relax/-relax setting.

[shiva0217]

In D44888#1109361, @asb wrote:

This is looking good to me, just needs an update to address this request for a test in riscv-features.c that demonstrates the default +relax/-relax setting.

Hi Alex. I added the testing line on D47127 which is the patch we turn on relaxation as default. Do you think it's ok?

[asb]

In D44888#1111920, @shiva0217 wrote:

In D44888#1109361, @asb wrote:

This is looking good to me, just needs an update to address this request for a test in riscv-features.c that demonstrates the default +relax/-relax setting.

Hi Alex. I added the testing line on D47127 which is the patch we turn on relaxation as default. Do you think it's ok?

I saw that. I don't mean to be too nit-picky, but there's an advantage in also testing the default in this patch - it tests the current behaviour and makes it obvious in the follow-up patch that the behaviour actually changed.

[shiva0217]

In D44888#1111995, @asb wrote:

In D44888#1111920, @shiva0217 wrote:

In D44888#1109361, @asb wrote:

This is looking good to me, just needs an update to address this request for a test in riscv-features.c that demonstrates the default +relax/-relax setting.

Hi Alex. I added the testing line on D47127 which is the patch we turn on relaxation as default. Do you think it's ok?

I saw that. I don't mean to be too nit-picky, but there's an advantage in also testing the default in this patch - it tests the current behaviour and makes it obvious in the follow-up patch that the behaviour actually changed.

OK, make sense to me.

[asb]

Looks good to me, thanks!

cfe/trunk/include/clang/Driver/Options.td

 def m_mips_Features_Group : OptionGroup<"<mips features group>">,
                             Group<m_Group>, DocName<"MIPS">;
 def m_ppc_Features_Group : OptionGroup<"<ppc features group>">,
                            Group<m_Group>, DocName<"PowerPC">;
 def m_wasm_Features_Group : OptionGroup<"<wasm features group>">,
                             Group<m_Group>, DocName<"WebAssembly">;
 def m_x86_Features_Group : OptionGroup<"<x86 features group>">,
                            Group<m_Group>, Flags<[CoreOption]>, DocName<"X86">;
+def m_riscv_Features_Group : OptionGroup<"<riscv features group>">,
+                             Group<m_Group>, DocName<"RISCV">;
 
 def m_libc_Group : OptionGroup<"<m libc group>">, Group<m_mips_Features_Group>,
                    Flags<[HelpHidden]>;
 
 def O_Group : OptionGroup<"<O group>">, Group<CompileOnly_Group>,
               DocName<"Optimization level">, DocBrief<[{
 Flags controlling how much optimization should be performed.}]>;
 

 def mno_pascal_strings : Flag<["-"], "mno-pascal-strings">,
   Alias<fno_pascal_strings>;
 def mno_red_zone : Flag<["-"], "mno-red-zone">, Group<m_Group>;
 def mno_relax_all : Flag<["-"], "mno-relax-all">, Group<m_Group>;
 def mno_rtd: Flag<["-"], "mno-rtd">, Group<m_Group>;
 def mno_soft_float : Flag<["-"], "mno-soft-float">, Group<m_Group>;
 def mno_stackrealign : Flag<["-"], "mno-stackrealign">, Group<m_Group>;
 
+def mrelax : Flag<["-"], "mrelax">, Group<m_riscv_Features_Group>,
+  HelpText<"Enable linker relaxation">;
+def mno_relax : Flag<["-"], "mno-relax">, Group<m_riscv_Features_Group>,
+  HelpText<"Disable linker relaxation">;
+
 def munaligned_access : Flag<["-"], "munaligned-access">, Group<m_arm_Features_Group>,
   HelpText<"Allow memory accesses to be unaligned (AArch32/AArch64 only)">;
 def mno_unaligned_access : Flag<["-"], "mno-unaligned-access">, Group<m_arm_Features_Group>,
   HelpText<"Force all memory accesses to be aligned (AArch32/AArch64 only)">;
 def mstrict_align : Flag<["-"], "mstrict-align">, Alias<mno_unaligned_access>, Flags<[CC1Option,HelpHidden]>,
   HelpText<"Force all memory accesses to be aligned (same as mno-unaligned-access)">;
 def mno_thumb : Flag<["-"], "mno-thumb">, Group<m_arm_Features_Group>;
 def mrestrict_it: Flag<["-"], "mrestrict-it">, Group<m_arm_Features_Group>,

cfe/trunk/lib/Driver/ToolChains/Arch/RISCV.cpp

 #include "RISCV.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/TargetParser.h"
 #include "llvm/Support/raw_ostream.h"
+#include "ToolChains/CommonArgs.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 static StringRef getExtensionTypeDesc(StringRef Ext) {
   if (Ext.startswith("sx"))
if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
 
     // Additional dependency checks.
     // TODO: The 'q' extension requires rv64.
     // TODO: It is illegal to specify 'e' extensions with 'f' and 'd'.
 
     // Handle all other types of extensions.
     getExtensionFeatures(D, Args, Features, MArch, OtherExts);
   }
+
+  // Now add any that the user explicitly requested on the command line,
+  // which may override the defaults.
+  handleTargetFeaturesGroup(Args, Features, options::OPT_m_riscv_Features_Group);
 }
 
 StringRef riscv::getRISCVABI(const ArgList &Args, const llvm::Triple &Triple) {
   if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ))
     return A->getValue();
 
   return Triple.getArch() == llvm::Triple::riscv32 ? "ilp32" : "lp64";
 }

cfe/trunk/test/Driver/riscv-features.c

 // RUN: %clang -target riscv32-unknown-elf -### %s -fsyntax-only 2>&1 | FileCheck %s
 // RUN: %clang -target riscv64-unknown-elf -### %s -fsyntax-only 2>&1 | FileCheck %s
 
 // CHECK: fno-signed-char
+
+// RUN: %clang -target riscv32-unknown-elf -### %s -mrelax 2>&1 | FileCheck %s -check-prefix=RELAX
+// RUN: %clang -target riscv32-unknown-elf -### %s -mno-relax 2>&1 | FileCheck %s -check-prefix=NO-RELAX
+// RUN: %clang -target riscv32-unknown-elf -### %s 2>&1 | FileCheck %s -check-prefix=DEFAULT
+
+// RELAX: "-target-feature" "+relax"
+// NO-RELAX: "-target-feature" "-relax"
+// DEFAULT-NOT: "-target-feature" "+relax"
+// DEFAULT-NOT: "-target-feature" "-relax"