[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

1879

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

lib/Driver/ToolChains/Arch/RISCV.cpp

55

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

69

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

69

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!

include/clang/Driver/Options.td

def m_hexagon_Features_HVX_Group : OptionGroup<"<hexagon features group>">,
                                    Group<m_Group>, DocName<"Hexagon">;
 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_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 DebugInfo_Group : OptionGroup<"<g group>">, Group<CompileOnly_Group>,
def mstack_arg_probe : Flag<["-"], "mstack-arg-probe">, Group<m_Group>,
   HelpText<"Enable stack probes">;
 def mno_stack_arg_probe : Flag<["-"], "mno-stack-arg-probe">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Disable stack probes which are enabled by default">;
 def mthread_model : Separate<["-"], "mthread-model">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"The thread model to use, e.g. posix, single (posix by default)">, Values<"posix,single">;
 def meabi : Separate<["-"], "meabi">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Set EABI type, e.g. 4, 5 or gnu (default depends on triple)">, Values<"default,4,5,gnu">;
 
+def mrelax : Flag<["-"], "mrelax">,
+    Group<m_riscv_Features_Group>;
+def mno_relax : Flag<["-"], "mno-relax">,
+    Group<m_riscv_Features_Group>;
+
 def mno_constant_cfstrings : Flag<["-"], "mno-constant-cfstrings">, Group<m_Group>;
 def mno_global_merge : Flag<["-"], "mno-global-merge">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Disable merging of globals">;
 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>;

lib/Driver/ToolChains/Arch/RISCV.cpp

 //===--- RISCV.cpp - RISCV Helpers for Tools --------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "RISCV.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;
 
 void riscv::getRISCVTargetFeatures(const Driver &D, const ArgList &Args,
                                    std::vector<StringRef> &Features) {
for (char c : MArchSplit.second) {
         Features.push_back("+d");
         break;
       case 'c':
         Features.push_back("+c");
         break;
       }
     }
   }
+
+  // -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");
+
+  // 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";
 }