[RISCV] Default enable linker relaxation and add -mrelax, -mno-relax flags
Needs RevisionPublic

Authored by shiva0217 on Mar 25 2018, 6:44 PM.

Details

Reviewers
asb
apazos
Summary

Default enable linker relaxation and add -mrelax, -mno-relax flags

Diff Detail

Repository
rL LLVM
shiva0217 created this revision.Mar 25 2018, 6:44 PM
apazos added inline comments.Tue, Mar 27, 11:07 AM
include/clang/Driver/Options.td
1886

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

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

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 retitled this revision from [RISCV] Default enable linker relaxation and add -mno-relax flag to disable it to [RISCV] Default enable linker relaxation and add -mrelax, -mno-relax flags.
shiva0217 edited the summary of this revision. (Show Details)

Update patch to address Ana's comments.

zzheng added a subscriber: zzheng.Tue, Apr 3, 12:05 PM
asb added a comment.Thu, Apr 5, 3:33 AM

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 added a comment.EditedThu, Apr 5, 3:36 AM
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 updated this revision to Diff 141591.Mon, Apr 9, 1:19 AM

Update patch to address Alex's comments.

asb added a comment.Thu, Apr 12, 6:07 AM

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

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

asb requested changes to this revision.Mon, Apr 16, 8:15 AM

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.

This revision now requires changes to proceed.Mon, Apr 16, 8:15 AM
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?