Page MenuHomePhabricator

[RISCV] Support resolving fixup_riscv_call and add to MCFixupKindInfo table
ClosedPublic

Authored by shiva0217 on May 21 2018, 12:23 AM.

Details

Summary

And also adding static_assert after array declaration
to avoid missing any new fixup in MCFixupKindInfo in the future.

Fixup fixup_riscv_call by assembler if the function and callsite within the same compile unit and the linker relaxation disabled.

The patch is according to
https://reviews.llvm.org/D44886#inline-411881

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.May 21 2018, 12:23 AM
asb added inline comments.May 23 2018, 4:55 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
51 ↗(On Diff #147736)

This comment should explain why we need to preserve the relocation type. e.g. "We are unable to correctly resolve a fixup_riscv_call, so always emit a relocation."

90 ↗(On Diff #147736)

Thinking about it, bits should really be 64 for fixup_riscv_call. With that in place, wouldn't it be feasible to add support to adjustFixupValue so we actually can resolve the fixup?

92 ↗(On Diff #147736)

You can just use array_lengthof(Infos).

94 ↗(On Diff #147736)

"Not all fixup kinds added to Infos array" is probably sufficient.

asb requested changes to this revision.May 23 2018, 4:56 AM
This revision now requires changes to proceed.May 23 2018, 4:56 AM
asb added inline comments.May 23 2018, 5:36 AM
test/CodeGen/RISCV/function-call.ll
3 ↗(On Diff #147736)

I think a comment explaining the purpose of the test would be useful. Presumably it's something like "Check that not attempt is made to resolve a call fixups and a relocation is always emitted". Of course this would be unnecessary if we could add support for correctly resolving call fixups!

shiva0217 updated this revision to Diff 148397.May 24 2018, 6:03 AM
shiva0217 edited the summary of this revision. (Show Details)

Hi Alex.It seems that fixup fixup_riscv_call by assembler is workable. Thanks for your comments.

asb added inline comments.May 28 2018, 4:37 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
280 ↗(On Diff #148397)

It would be helpful to have a comment to explain the addition of 0x800ULL.

test/CodeGen/RISCV/function-call.ll
1–3 ↗(On Diff #148397)

Now this test has changed to just checking the fixup is resolved when -mattr=-relax I think it makes much more sense as an MC layer test. Perhaps expanding test/MC/RISCV/fixups.s? We need some reassurance that the fixup calculation is correct, even for offsets > 12 bits.

It looks like you have to use .space as in fixups.s, as the following still results in a relocation, even though I would have expected it not to:

.equ addr, 0xdeadbeef
call addr
asb added inline comments.May 28 2018, 5:03 AM
test/CodeGen/RISCV/function-call.ll
1–3 ↗(On Diff #148397)

Sorry - I didn't turn my brain on for that comment. It's absolutely correct that the call to an absolute target results in a relocation, as of course call is pc-relative. Any way, more tests in test/MC/RISCV/fixups.s to give reassurance that resolvable fixups are encoded correctly would be really welcome.

shiva0217 updated this revision to Diff 148842.May 28 2018, 6:47 PM

Update test cases to address Alex's comments.

asb accepted this revision.May 29 2018, 12:12 PM

Thanks, looks good to me. Probably worth retitling the commit to e.g. "[RISCV] Support resolving fixup_riscv_call and add to MCFixupKindInfo table"

This revision is now accepted and ready to land.May 29 2018, 12:12 PM
shiva0217 retitled this revision from [RISCV] Add missing fixup_riscv_call in MCFixupKindInfo table to [RISCV] Support resolving fixup_riscv_call and add to MCFixupKindInfo table.May 29 2018, 6:09 PM
This revision was automatically updated to reflect the committed changes.