This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support assembling %got_pcrel_hi operator
ClosedPublic

Authored by jrtc27 on Dec 4 2018, 9:36 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jrtc27 created this revision.Dec 4 2018, 9:36 AM

This patch is a very reasonable one: it is a bit weird we can't expand la (under -fPIC) because R_RISCV_GOT_HI20 relocation does not have any associated syntax.

But I think we should go first to https://github.com/riscv/riscv-elf-psabi-doc (or the relevant forum) and suggest this new syntax before commiting this.

What do you think?

asb added a comment.Dec 4 2018, 10:59 AM

@rogfer01: James did actually get a patch for this committed to riscv-asm-manual https://github.com/riscv/riscv-asm-manual/blob/master/riscv-asm.md

Ah, thanks @asb definitely I was looking at the wrong place!

Ah, thanks @asb definitely I was looking at the wrong place!

Yeah, upstream suggested axing the list of assembly operators from the psABI doc as it's unnecessary duplication that gets out of date. Volunteers?...

I deliberately didn't include any expansion of la as the two can be done independently. I do however intend to submit a patch for it as well over the next few days, which shouldn't prove too problematic for D50496 as it already needs modifying and rebasing.

Yeah, upstream suggested axing the list of assembly operators from the psABI doc as it's unnecessary duplication that gets out of date. Volunteers?...

I may give it a shot then. At least a pointer to the asm manual will avoid confusions like mine in the future.

I deliberately didn't include any expansion of la as the two can be done independently. I do however intend to submit a patch for it as well over the next few days, which shouldn't prove too problematic for D50496 as it already needs modifying and rebasing.

Thanks a lot for working on this!

lewis-revill requested changes to this revision.Dec 5 2018, 9:45 AM
lewis-revill added a subscriber: lewis-revill.

Could you correct the error message for InvalidUImm20AUIPC in RISCVAsmParser to include this new operator & change rv32i-invalid.s appropriately? When I did this I also added a test line in rv32i-valid.s but it's mostly testing the same thing as the test in relocations.s. Maybe you want to add that as well for completeness though.

This revision now requires changes to proceed.Dec 5 2018, 9:45 AM
jrtc27 added a comment.Dec 6 2018, 5:47 AM

Could you correct the error message for InvalidUImm20AUIPC in RISCVAsmParser to include this new operator & change rv32i-invalid.s appropriately? When I did this I also added a test line in rv32i-valid.s but it's mostly testing the same thing as the test in relocations.s. Maybe you want to add that as well for completeness though.

Ah thanks for catching that.

jrtc27 added a comment.Dec 6 2018, 5:52 AM

I realise now this is also dependent on D54029, otherwise the corresponding %pcrel_lo gets evaluated to the offset of the auipc. I'll rebase on top of that.

jrtc27 updated this revision to Diff 177217.Dec 7 2018, 7:41 AM

Rebased on top of D54029, added test case to ensure R_RISCV_GOT_HI20 is always preserved for the linker and updated error message for InvalidUImm20AUIPC.

lewis-revill requested changes to this revision.Dec 10 2018, 7:14 PM

Do you need to add a check for RISCV::fixup_riscv_pcrel_hi20 in getPcRelHiExpr? Currently this is returning null and causing a segfault when running your relocations.s tests.

This revision now requires changes to proceed.Dec 10 2018, 7:14 PM

Do you need to add a check for RISCV::fixup_riscv_pcrel_hi20 in getPcRelHiExpr? Currently this is returning null and causing a segfault when running your relocations.s tests.

RISCV::fixup_riscv_pcrel_hi20 ->RISCV::fixup_riscv_got_hi20

lewis-revill added inline comments.Dec 10 2018, 7:24 PM
test/MC/RISCV/relocations.s
85 ↗(On Diff #177217)

These tests are failing for me? I am testing this on top of master & D54029

Do you need to add a check for RISCV::fixup_riscv_pcrel_hi20 in getPcRelHiExpr? Currently this is returning null and causing a segfault when running your relocations.s tests.

RISCV::fixup_riscv_pcrel_hi20 ->RISCV::fixup_riscv_got_hi20

Yes, but in D54029 I had asked that check to be included (I just patched D54029 locally). With the latest version of that revision, though, it's going to print out an error about not being able to find the corresponding HI relocation so needs some further changes.

PkmX added a comment.Dec 11 2018, 6:14 AM

Yes, getPCRelHiExpr from D54029 needs to modified to also look for fixup_riscv_got_hi20, and if it does find one it should just force a relocation.

I think that is a change that should be implemented in this revision, since this is the revision that adds the fixup and the tests. Otherwise we would end up with D54029 depending on this, which itself depends on D54029.

jrtc27 updated this revision to Diff 177721.Dec 11 2018, 8:35 AM

Rebased on top of latest changes to D54029

jrtc27 marked an inline comment as done.Dec 11 2018, 8:38 AM

Slight annoying bit of churn turning getPCRelHiExpr into getPCRelHiFixup (when I tweaked D54029 I simply didn't make shouldForceRelocation give an error for a null getPCRelHiExpr return value, and therefore it could remain unchanged, but with the increased error checking we now need to get the entire MCFixup back).

asb accepted this revision.Dec 20 2018, 6:57 AM

This no longer seems to apply cleanly (needs rebase), but otherwise LGTM. Thanks!

rogfer01 added inline comments.Dec 21 2018, 2:54 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
51 ↗(On Diff #177721)

Should this error message be updated to include %got_pcrel_hi too?

If so, I think we will have to update it again for the forthcoming %tls_ie_pcrel_hi and %tls_gd_pcrel_hi (not in this change) but in D55342.

Can this be rebased to apply cleanly with the upstreamed D54029?

asb requested changes to this revision.Jan 22 2019, 11:26 PM

Hi James, can you please rebase and confirm that you are aware of the new license put in place last Friday and intend to contribute under it. See http://lists.llvm.org/pipermail/llvm-dev/2019-January/129344.html https://llvm.org/docs/DeveloperPolicy.html#new-llvm-project-license-framework

This revision now requires changes to proceed.Jan 22 2019, 11:26 PM
jrtc27 updated this revision to Diff 185316.EditedFeb 5 2019, 8:28 AM

Rebased (sorry for the delay).

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 8:28 AM
jrtc27 added a comment.Feb 5 2019, 8:30 AM
In D55279#1367361, @asb wrote:

Hi James, can you please rebase and confirm that you are aware of the new license put in place last Friday and intend to contribute under it. See http://lists.llvm.org/pipermail/llvm-dev/2019-January/129344.html https://llvm.org/docs/DeveloperPolicy.html#new-llvm-project-license-framework

Hi Alex, yes, I'm happy contributing under the new license (and filled out the relicensing form a few weeks ago too).

asb accepted this revision.Feb 12 2019, 8:02 AM

Thanks, this looks good to me (see small nit in comment). Do you need me to commit?

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
194 ↗(On Diff #185316)

Given that we expect a relocation will always be emitted, perhaps we should add instead:

case RISCV::fixup_riscv_got_hi20:
  llvm_unreachable("Relocation should have been forced for fixup")
asb added a comment.Feb 14 2019, 7:13 AM
In D55279#1394818, @asb wrote:

Thanks, this looks good to me (see small nit in comment). Do you need me to commit?

Hi James - would be great to get this in. Just a ping on this question.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 15 2019, 1:44 AM
This revision was automatically updated to reflect the committed changes.