[RISCV] Implement GOT load relaxation#1270
Draft
Jonathon Penix (jonathonpenix) wants to merge 5 commits into
Draft
[RISCV] Implement GOT load relaxation#1270Jonathon Penix (jonathonpenix) wants to merge 5 commits into
Jonathon Penix (jonathonpenix) wants to merge 5 commits into
Conversation
Jonathon Penix (jonathonpenix)
force-pushed
the
pr/riscv_got_relax
branch
2 times, most recently
from
edad330 to
89c068f
Compare
This code should be dead as `R_RISCV_RVC_LUI` is an internal relocation to help with relaxation and the only place we emit this relocation already checks that the immediate is non-zero. Cleaning this up is also helpful as a follow up patch will introduce a new internal relocation for `c.li` (for handling GOT load relaxations). The existing code is incorrect for that case, and cleaning this up will let us handle these two consistently (and more consistently with other instruction types). Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
…Shift`. NFCI A follow-up patch will add a new internal relocation to handle `c.li`. Currently, `encodeCI` looks for the immediate in the higher bits of `Result` (bits 17 and 16:12) which makes sense for `c.lui` handling but not really for `c.li` (where we want to look at bits 5 and 4:0). I don't think this would work as-is for some of the other CI-type instructions if they ever need to be handled so it isn't entirely generic. But, this should be sufficient for supporting both `c.li` and `c.lui`. I also think this is marginally simpler/more straightforward than creating a separate `EncTy_*` or checking the instruction type, etc. so I went this route for now. Note `isValidRVCLUIType` is now in a (more) broken state as I didn't update the call to `encodeCI`. As far as I can tell, this code is already dead/broken (it ultimately calls `extractRVCImmediate` which itself has a typo (`exractBits`) so has never been compile-able as far as I can tell. Not sure if we want to fix this up or just remove it, so I'm leaving it be for the purposes of this patch. Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
The relaxation is defined in the RISC-V psABI at [1]. Broadly, it allows relaxing a GOT-relative sequence to either a PC-relative sequence or absolute addi/c.li. There's a few limitations or items left for future work: - The psABI allows the linker to omit the GOT slot for a symbol if all references to the symbol's GOT slot is removed. This is not implemented. - Similar to other relaxations in eld, relaxations for symbols of value 0 are generally avoided. - Weak undef symbols cannot be relaxed currently under `-static -pie` as they are considered preemptible still. It's not clear to me if this intended (and, we do have `-static -pie` users so it might be worth cleaning this up if possible)--this is left to resolve elsewhere. - All PCREL_LO12s must be at a higher offset than the GOT_HI20 they refer to for the relaxation to kick in. This isn't required by the psABI as far as I'm aware, but there's a bug that can cause incorrect relaxations if this is not the case. My assumption is the vast majority of normal usecase will meet this requirement, so addressing this is left as follow-up work. See qualcomm#1276 - This implementation is a bit buggy in that it can incorrectly relax if next to deleted instructions that were marked relax. See qualcomm#1278. Leaving this to be fixed in the same manner as 1278. [1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/ffde5a7f62a1bb12a4d775afa45342ac24138849/riscv-elf.adoc?plain=1#L1774 Assisted by Claude. Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
Jonathon Penix (jonathonpenix)
force-pushed
the
pr/riscv_got_relax
branch
from
89c068f to
c279882
Compare
Jonathon Penix (jonathonpenix)
requested a review
from Shankar Easwaran (quic-seaswara)
as a code owner
Contributor
Author
|
There's an XPASS on this currently that I still need to look into. But, in the meantime any initial feedback would be greatly appreciated! |
This was referenced Jun 25, 2026
…avior Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
| #END_COMMENT | ||
| #START_TEST | ||
| RUN: %run_cc -o %t1.o %p/Inputs/HelloWorld.c -c -fno-pic | ||
| RUN: %run_cc -o %t.norelax.out %t1.o -static -Wl,--no-relax-got |
Contributor
Author
There was a problem hiding this comment.
Adding this as the test otherwise unexpectedly passes with this patch (it's xfail'ed for RISC-V currently). AFAICT the PR here is working as intended, it just happens to paper over an unrelated bug. See #1342
So I think it is worth covering both relax and no-relax cases rather than just un-XFAIL'ing the test or something.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The relaxation is defined in the RISC-V psABI at [1].
Broadly, it allows relaxing a GOT-relative sequence to either a PC-relative
sequence or absolute addi/c.li.
There's a few limitations or items left for future work:
references to the symbol's GOT slot is removed. This is not implemented.
0 are generally avoided.
-static -pieas theyare considered preemptible still. It's not clear to me if this intended
(and, we do have
-static -pieusers so it might be worth cleaning thisup if possible)--this is left to resolve elsewhere.
to for the relaxation to kick in. This isn't required by the psABI as
far as I'm aware, but there's a bug that can cause incorrect relaxations
if this is not the case. My assumption is the vast majority of normal
usecase will meet this requirement, so addressing this is left as
follow-up work. See eld reorders
PCREL_LO12_*/RELAXrelocs when the LO is a forward reference #1276if next to deleted instructions that were marked relax. See
TLSDESC relaxations can incorrectly fire if they're next to a instruction marked
RELAXthat will be deleted #1278. Leaving this to be fixedin the same manner as 1278.
[1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/ffde5a7f62a1bb12a4d775afa45342ac24138849/riscv-elf.adoc?plain=1#L1774
Assisted by Claude.
======
Stacked on #1268 and #1269