Page MenuHomePhabricator

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

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

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Mar 25 2018, 6:44 PM
apazos added inline comments.Mar 27 2018, 11:07 AM
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 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.Apr 3 2018, 12:05 PM
asb added a comment.Apr 5 2018, 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.EditedApr 5 2018, 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.Apr 9 2018, 1:19 AM

Update patch to address Alex's comments.

asb added a comment.Apr 12 2018, 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.Apr 16 2018, 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.Apr 16 2018, 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?

kito-cheng added inline comments.May 13 2018, 8:14 PM
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 updated this revision to Diff 146737.May 14 2018, 6:58 PM

Rebase patch to D45465

shiva0217 added inline comments.May 14 2018, 7:00 PM
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 added a comment.May 17 2018, 3:23 AM

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.

shiva0217 updated this revision to Diff 147739.May 21 2018, 1:03 AM
shiva0217 retitled this revision from [RISCV] Default enable linker relaxation and add -mrelax, -mno-relax flags to [RISCV] Add -mrelax/-mno-relax flags to enable/disable RISCV linker relaxation.
shiva0217 edited the summary of this revision. (Show Details)
asb added a comment.May 23 2018, 5:45 AM

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.

asb requested changes to this revision.May 23 2018, 5:46 AM
This revision now requires changes to proceed.May 23 2018, 5:46 AM
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 added a comment.May 24 2018, 11:22 PM
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.

In D44888#1111995, @asb 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 accepted this revision.May 28 2018, 6:33 AM

Looks good to me, thanks!

This revision is now accepted and ready to land.May 28 2018, 6:33 AM
This revision was automatically updated to reflect the committed changes.