This is an archive of the discontinued LLVM Phabricator instance.

[MC][CodeGen] Define R_RISCV_PLT32 and lower dso_local_equivalent to it
ClosedPublic

Authored by leonardchan on Feb 2 2023, 3:51 PM.

Details

Summary

This introduces R_RISCV_PLT32, PC-relative data relocation that takes the 32-bit relative offset to a function or its PLT entry from its relocation location.

This is needed to support relative vtables on RISCV.

Github PR: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/363

The lld handling of this reloc is D143115.

Diff Detail

Event Timeline

leonardchan created this revision.Feb 2 2023, 3:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 3:51 PM
leonardchan requested review of this revision.Feb 2 2023, 3:51 PM
mcgrathr accepted this revision.Feb 2 2023, 5:21 PM

LGTM pending acceptance of the spec change.

llvm/test/MC/RISCV/elf-reloc-plt32.s
6

This label is unused.

This revision is now accepted and ready to land.Feb 2 2023, 5:21 PM

Title should say [MC] not [llvm]. Do we not also need CodeGen support for this to be useful outside of hand-written assembly?

llvm/test/MC/RISCV/elf-reloc-plt32.s
2–3
MaskRay added inline comments.Feb 2 2023, 5:46 PM
llvm/test/MC/RISCV/elf-reloc-plt32.s
2–3

This is a difference in taste. In MC and lld tests, backslash at the previous line is pretty common.

jrtc27 added inline comments.Feb 2 2023, 5:49 PM
llvm/test/MC/RISCV/elf-reloc-plt32.s
2–3

In a tree I have lying around from August there are only 4 instances of \| \\$ in llvm/test/MC/RISCV but 1088 of : *\|

leonardchan marked 4 inline comments as done.
leonardchan retitled this revision from [llvm][RISCV] Introduce handling for R_RISCV_PLT32 relocation to [MC][RISCV] Introduce handling for R_RISCV_PLT32 relocation.

Title should say [MC] not [llvm]. Do we not also need CodeGen support for this to be useful outside of hand-written assembly?

CodeGen already picks this up via the PLTRelativeVariantKind = MCSymbolRefExpr::VK_PLT;. A dso_local_equivalent @func gets lowered to func@PLT.

jrtc27 added a comment.Feb 3 2023, 2:09 PM

Title should say [MC] not [llvm]. Do we not also need CodeGen support for this to be useful outside of hand-written assembly?

CodeGen already picks this up via the PLTRelativeVariantKind = MCSymbolRefExpr::VK_PLT;. A dso_local_equivalent @func gets lowered to func@PLT.

Then this should be [MC][CodeGen] and have an IR test for it

leonardchan retitled this revision from [MC][RISCV] Introduce handling for R_RISCV_PLT32 relocation to [MC][CodeGen] Introduce handling for R_RISCV_PLT32 relocation.

Title should say [MC] not [llvm]. Do we not also need CodeGen support for this to be useful outside of hand-written assembly?

CodeGen already picks this up via the PLTRelativeVariantKind = MCSymbolRefExpr::VK_PLT;. A dso_local_equivalent @func gets lowered to func@PLT.

Then this should be [MC][CodeGen] and have an IR test for it

Done

jrtc27 added inline comments.Feb 3 2023, 2:30 PM
llvm/test/CodeGen/RISCV/dso_local_equivalent.ll
1 ↗(On Diff #494737)

Use update_llc_test_checks.py

4 ↗(On Diff #494737)

If we're producing assembly that we test in an MC test we don't need this. Generally best to not do object file tests in CodeGen.

9 ↗(On Diff #494737)

This isn't PC-relative

leonardchan marked 3 inline comments as done.
leonardchan added inline comments.
llvm/test/CodeGen/RISCV/dso_local_equivalent.ll
9 ↗(On Diff #494737)

Updated with pc-relative codegen

jrtc27 added inline comments.Feb 3 2023, 3:19 PM
llvm/test/CodeGen/RISCV/dso_local_equivalent.ll
1–11 ↗(On Diff #494746)

Hm, seems update_llc_test_checks.py can't handle globals, just uses an empty dict :(

In that case I think you want this cleaned up version of your hand-written test.

Ideally we'd also have an RV32 test, but unifying them is a bit awkward due to the trunc needing to not exist. Technically you can do an oversized ptrtoint on RV32 (or undersized ptrtoint on RV64 and forego the trunc on both) but that's not what Clang (currently) generates. I do wonder why it doesn't just do an i32 ptrtoint unconditionally though...

llvm/test/MC/RISCV/elf-reloc-plt32.s
5

Just .data?

leonardchan marked 2 inline comments as done.
MaskRay accepted this revision.Feb 3 2023, 3:32 PM

This introduces R_RISCV_PLT32, a pc-relative relocation that takes the offset to a function (or its plt entry) from the reloc's location.

My suggested wording on the PR uses proper cases: PC-relative and PLT, and relocation location.

llvm/test/CodeGen/RISCV/dso_local_equivalent.ll
6 ↗(On Diff #494758)

You can add {{$}} to the end to prevent such a mistake or switch to FileCheck --match-full-lines

1 ↗(On Diff #494737)

I think it is fine not using update_llc_test_checks.py. The test clearly doesn't cause more maintenance trouble.

This revision now requires review to proceed.Feb 3 2023, 3:32 PM
jrtc27 added inline comments.Feb 3 2023, 3:33 PM
llvm/test/CodeGen/RISCV/dso_local_equivalent.ll
6 ↗(On Diff #494758)

Sorry, my bad, I meant to say CHECK-NEXT here but forgot to alter what I copied...

MaskRay added a comment.EditedFeb 3 2023, 3:35 PM

A subject like Define R_RISCV_PLT32 and lower dso_local_equivalent to it may be clearer than Introduce handling for R_RISCV_PLT32 relocation

leonardchan edited the summary of this revision. (Show Details)Feb 3 2023, 3:37 PM
leonardchan retitled this revision from [MC][CodeGen] Introduce handling for R_RISCV_PLT32 relocation to [MC][CodeGen] Define R_RISCV_PLT32 and lower dso_local_equivalent to it.
leonardchan marked 3 inline comments as done.
MaskRay accepted this revision.Feb 3 2023, 3:54 PM
asb added a comment.Feb 7 2023, 2:49 AM

I wanted to raise something that came to mind looking at this patch, but is probably also relevant for the other recent Fuchsia related patches: typically (certainly for the llvm/test/CodeGen/RISCV and llvm/test/MC/RISCV) we try to add RV32 CHECK lines alongside RV64 lines wherever relevant. This is typically cheap to do, and of course provides just a little extra coverage, even in cases where the test isn't particularly targeted at an RV32 use case.

So the two thoughts are:

  • Adding rv32 lines to tests like in this patch
  • Whether the same should be done for other fuchsia related patches, and relatedly if -mtriple=riscv32-fuchsia is something that isn't likely to exist and we don't want to bother testing for it, would it be better to just disallow the triple?

I wanted to raise something that came to mind looking at this patch, but is probably also relevant for the other recent Fuchsia related patches: typically (certainly for the llvm/test/CodeGen/RISCV and llvm/test/MC/RISCV) we try to add RV32 CHECK lines alongside RV64 lines wherever relevant. This is typically cheap to do, and of course provides just a little extra coverage, even in cases where the test isn't particularly targeted at an RV32 use case.

So the two thoughts are:

  • Adding rv32 lines to tests like in this patch
  • Whether the same should be done for other fuchsia related patches, and relatedly if -mtriple=riscv32-fuchsia is something that isn't likely to exist and we don't want to bother testing for it, would it be better to just disallow the triple?

I'm fine with adding RV32 cases in this patch and other patches related to this if it's trivial. For fuchsia-related changes though, I think the 32-bit tests might not be necessary since Fuchsia is exclusively 64-bit so we could disallow that in a separate patch maybe.

asb added a comment.Feb 9 2023, 11:22 AM

For fuchsia-related changes though, I think the 32-bit tests might not be necessary since Fuchsia is exclusively 64-bit so we could disallow that in a separate patch maybe.

That would be great. There are enough valid/expected options to worry about that I'm in quite favour of erroring out early for things that we don't think make sense.

@jrtc27 any comments on this before landing?

This revision was not accepted when it landed; it landed in state Needs Review.Feb 22 2023, 5:28 PM
This revision was automatically updated to reflect the committed changes.
asb added a comment.Feb 28 2023, 5:57 AM

For fuchsia-related changes though, I think the 32-bit tests might not be necessary since Fuchsia is exclusively 64-bit so we could disallow that in a separate patch maybe.

That would be great. There are enough valid/expected options to worry about that I'm in quite favour of erroring out early for things that we don't think make sense.

Just a friendly ping on this - follow-up patches to gate off rv32 fuchsia (and any other functionality introduced for fuchsia but isn't expected to work on rv32) would be appreciated.

Just a friendly ping on this - follow-up patches to gate off rv32 fuchsia (and any other functionality introduced for fuchsia but isn't expected to work on rv32) would be appreciated.

Sorry for the delay. D144998 adds this.