Page MenuHomePhabricator

[RISCV] Support linker relax function call from auipc and jalr to jal
ClosedPublic

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

Details

Summary

To do this:

  1. Add fixup_riscv_relax fixup types which eventually will transfer to R_RISCV_RELAX relocation type.
  1. Insert R_RISCV_RELAX relocation type to auipc function call expression when linker relaxation enabled.

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Mar 25 2018, 6:30 PM
apazos added inline comments.Mar 28 2018, 4:32 PM
test/CodeGen/RISCV/linker-relaxation.ll
1 ↗(On Diff #139755)

if you decide to add the no-relax flag, maybe you can add the test case for that too.

shiva0217 updated this revision to Diff 140176.Mar 28 2018, 6:22 PM

Update patch to address Ana's comments.

zzheng added a subscriber: zzheng.Apr 3 2018, 12:05 PM
asb added inline comments.Apr 5 2018, 3:24 AM
lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h
50 ↗(On Diff #140176)

The description indicates you'd only want to use the call fixup when relaxation is enabled. But there's nothing stopping us from using fixup_riscv_call even when linker relaxation is disabled - it reduces the number of relocations for the linker to process.

How about something like the following, which better explains its use "A fixup representing a call attached to the auipc instruction of an adjacent auipc+pair."

52 ↗(On Diff #140176)

Maybe something like "Used to generate an R_RISCV_RELAX relocation type, which indicates the linker may relax the instruction pair."

test/CodeGen/RISCV/linker-relaxation.ll
8 ↗(On Diff #140176)

I think we need to test the assembly output here, which will show that %pcrel_lo/%pcrel_hi are still emitted (and also checks that the asm emitter code doesn't crash when encountering VK_RISCV_CALL_LO). It's obviously unfortunate that generating .s and then assembling that will produce a different result (two relocations) than emitting the .o directly. Emitting the 'call` pseudoinstruction as used by gcc would fix that issue, but still leaves you with no way of expressing the "expanded" auipc+jalr form.

asb added a comment.Apr 5 2018, 3:48 AM

Could you please add tests that demonstrate the new logic in shouldForceRelocation? Specifically, it would be good to show that relocations are emitted even for local control flow, when they weren't before.

shiva0217 updated this revision to Diff 141597.EditedApr 9 2018, 1:55 AM

Update patch to address Alex's comments.

shiva0217 added inline comments.Apr 9 2018, 2:07 AM
test/CodeGen/RISCV/linker-relaxation.ll
8 ↗(On Diff #140176)

Hi Alex. Should we emit the call pseudo instruction as gcc to fix the issue? To do that, we could delay the function call expansion to encoding to preserve call.

asb added a comment.Apr 12 2018, 6:06 AM

This seems like a sensible intermediate step, even if it's not a 100% match for gcc behaviour.

I note that assembling asm generated by linker-relaxation.ll results in a RISCV_RELAX being generated with gas attached to both the pcrel_hi20 and the pcrel_lo12_i. I think we definitely want to do the same thing and it probably makes sense to do so before switching to directly generating the RISCV_CALL relocation - do you think?

test/CodeGen/RISCV/linker-relaxation.ll
8 ↗(On Diff #140176)

I think in the future we'll want to emit the call pseudoinstruction unless -mexplicit-relocs is set. gcc (at least the version I have built) still seems to emit the call pseudoinstruction when -mexplicit-relocs is set, which seems incorrect to me (though maybe I'm misunderstanding that compiler option).

asb added a comment.Apr 12 2018, 6:22 AM

Given that Simon's D45181 will be dependent on this (as it should really query whether linker relaxation is enabled), it probably makes sense to get this merged sooner rather than later.

I think the options are:

  1. Merge as is
  2. Modify this patch so it adds RISCV_RELAX to pcrel_hi and pcrel_lo relocations when linker relaxation is enabled, then have a follow-up patch that emits CALL.

The psabi doc doesn't discuss attaching RISCV_RELAX to relocations other than RISCV_CALL or RISCV_CALL_PLT, but presumably binutils ld will use it for other pairs?

shiva0217 updated this revision to Diff 142342.EditedApr 13 2018, 12:22 AM
shiva0217 edited the summary of this revision. (Show Details)

Update patch to reflect the updated in https://reviews.llvm.org/D44885.

In D44886#1065531, @asb wrote:

Given that Simon's D45181 will be dependent on this (as it should really query whether linker relaxation is enabled), it probably makes sense to get this merged sooner rather than later.

Hi Alex.

I think the options are:

  1. Merge as is

If we merge as is will leave the relocation difference between assembly code and object file as you described.

  1. Modify this patch so it adds RISCV_RELAX to pcrel_hi and pcrel_lo relocations when linker relaxation is enabled, then have a follow-up patch that emits CALL.

The psabi doc doesn't discuss attaching RISCV_RELAX to relocations other than RISCV_CALL or RISCV_CALL_PLT, but presumably binutils ld will use it for other pairs?

The psabi doc didn't mention add RISCV_RELAX to pcrel_hi and pcrel_lo relocations and I have done some experiments. It seems that attaching RISCV_RELAX to pcrel_hi and pcrel_lo relocations can't relax aupic and jalr to jal by Binutils ld. So we have to use R_RISCV_CALL and R_RISCV_RELAX for function call relaxation.

I have updated the patch to generate call pseudoinstruction as your comments in https://reviews.llvm.org/D44886?id=140176#inline-398646.

shiva0217 updated this revision to Diff 144833.May 1 2018, 10:22 PM

rebase patch to current trunk

asb added a comment.May 9 2018, 9:49 PM

Hi Shiva. Could you please split out the definition of shouldForceRelocation, enableLinkerRelax and FeatureRelax to a separate patch. That way we can commit that followed by all the patches that rely on querying whether linker relaxation is enabled, and then finally enable it via the RISCVMCCodeEmitter changes in this patch. Now there are a few patches we know we want to commit before linker relaxation is actually safe to use, I think that's probably the best way to structure things.

shiva0217 updated this revision to Diff 146071.May 9 2018, 11:44 PM
shiva0217 edited the summary of this revision. (Show Details)

Split ShouldForceRelocation+FeatureRelax into a sperate patch for better structure patches as Alex's suggestion.

shiva0217 updated this revision to Diff 146089.May 10 2018, 1:36 AM

Add fixup_riscv_call and fixup_riscv_relax to getFixupKindInfo table.

shiva0217 updated this revision to Diff 146092.May 10 2018, 1:48 AM

update testcase

asb added a comment.May 17 2018, 3:08 AM

Thanks Shiva, a few minor comments but with those resolved it should be good to go.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
77 ↗(On Diff #146092)

Good catch that this wasn't present. Given that riscv_call refers to an auipc+jalr shouldn't this be tagged MCFixupKindInfo::FKF_IsPCRel?

The assert that's in this function isn't sufficient, so to avoid missing these fixups in the future, could you change the Infos line to const static MCFixupKindInfo Infos[] and after the array add a static assert like const static MCFixupKindInfo Infos[RISCV::NumTargetFixupKinds].

Feel free to commit this fix directly, as it doesn't really need to be part of this patch (and even if we reverted this patch for some reason, we'd still want to keep the fixup_riscv_call fix).

78 ↗(On Diff #146092)

Shouldn't bits = 0 for relax, given that it's purely a tag?

test/MC/RISCV/linker-relaxation.s
4 ↗(On Diff #146092)

Might be clearer as NORELAX-RELOC

7 ↗(On Diff #146092)

Could you please add some riscv64 RUN lines

12 ↗(On Diff #146092)

We should check NORELAX-RELOC-NOT: R_RISCV_RELAX

asb requested changes to this revision.May 17 2018, 3:09 AM
This revision now requires changes to proceed.May 17 2018, 3:09 AM
shiva0217 added inline comments.May 20 2018, 7:51 PM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
77 ↗(On Diff #146092)

Hi Alex. If we tag fixup_riscv_call with FKF_IsPCRel, Assembler will try to applyFixup when the function and the call site within the same compile unit. So when relaxation disabled, shouldForceRelocation return false, will trigger "Unknown fixup kind!" in adjustFixupValue function. I think we should define as {fixup_riscv_call, 0, 32, 0 } or {fixup_riscv_call", 0, 0, 0 } to indicate that the fixup should never apply. Another solution is define as {fixup_riscv_call, 0, 32, MCFixupKindInfo::FKF_IsPCRel } but always return true in shouldForceRelocation if the fixup is fixup_riscv_call. What do you think?

shiva0217 added inline comments.May 21 2018, 12:32 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
77 ↗(On Diff #146092)

Hi Alex. I create a revision D47126 for this with a test case. It would be easier to observe the issue and the RISCV developers could apply the patch before we commit to trunk.

shiva0217 updated this revision to Diff 147769.May 21 2018, 5:06 AM

Update the patch to address Alex's comments.

asb accepted this revision.May 23 2018, 6:01 AM

This looks good to me. I'd say this could land prior to the R_RISCV_ALIGN patch if you rebased it appropriately.

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