This is an archive of the discontinued LLVM Phabricator instance.

[lld][RISCV] Implement GP relaxation for R_RISCV_HI20/R_RISCV_LO12_I/R_RISCV_LO12_S.
ClosedPublic

Authored by craig.topper on Feb 9 2023, 12:32 PM.

Details

Summary

This implements support for relaxing these relocations to use the GP
register to commute addresses of globals in the .sdata and .sbss
sections.

This feature is off by default and must be enabled by passing
--relax-gp to the linker.

The GP register might not always be the "global pointer". It can
be used for other purposes. See discussion here
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/371

Diff Detail

Event Timeline

craig.topper created this revision.Feb 9 2023, 12:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2023, 12:32 PM
craig.topper requested review of this revision.Feb 9 2023, 12:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2023, 12:32 PM

Take part of the test from D100835

git add the test file

MaskRay added a comment.EditedFeb 9 2023, 1:54 PM

Thanks for the patch! Making GP relaxation an opt-in feature is a good approach to me considering https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/298 and https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/305.
The option name I think of is --[no-]relax-gp. I even posted a GNU ld patch https://sourceware.org/pipermail/binutils/2022-July/121787.html but got a confusing reply unsure how I should act upon.
I wish that GNU ld is willing to adopt an option as well, even if defaulting to GP relaxation (hopefully real needs enlightened by https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/298 and 305 can change their minds).

MaskRay added inline comments.Feb 9 2023, 1:57 PM
lld/ELF/Arch/RISCV.cpp
443

remove

635

const. Actually avoid this used-once variable

637

early return

lld/ELF/Options.td
237

Update lld/docs/ld.lld.1

Use BB to support no-. Don't add single-dash long options for new options.

Address review comments

craig.topper marked 4 inline comments as done.Feb 9 2023, 3:02 PM
craig.topper added inline comments.Feb 9 2023, 3:05 PM
lld/ELF/Writer.cpp
977

@MaskRay should I add RISC-V constants in the enum near MIPS and PowerPC and push all other constants up by 2? Or should I add RISC-V constants that overlap with Mips or PowerPC?

craig.topper edited the summary of this revision. (Show Details)
jrtc27 added inline comments.Feb 9 2023, 3:10 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
49

These don't exist, they were removed as they were solely internal to binutils, and their encodings are reserved for future standard use

craig.topper added inline comments.Feb 9 2023, 3:13 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
49

Is binutils only going to be updated to use different values when a new standard use is introduced?

Is there a safe number to use for LLD insternal use?

jrtc27 added inline comments.Feb 9 2023, 3:23 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
49

So long they're not permitted in inputs it's "fine" as it won't misinterpret future files, but using ELF_RELOC here means all the other tools will think this is a real relocation (e.g. llvm-readelf will decode it). Can you not just use R_RISCV_RELAX like for TLS, though? It seems you're doing the exact same thing.

craig.topper added inline comments.Feb 9 2023, 3:27 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
49

In order to support .sbss, I believe I need to create a relocation to communicate from the instruction relaxation phase to the relax function which runs later. When I tried to rewrite .sbss globals in relaxHi20Lo12 I got the wrong addresses. My thinking was that I needed the .text section to shrink and the layout of later sections to adjust before I could do the instruction rewriting.

Add section rank flags for RISC-V.

craig.topper added inline comments.Feb 13 2023, 1:29 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
49

Since R_RISCV_GPREL_I/R_RISCV_GPREL_S are in the normal relocation list in binutils, doesn't that mean binutils readelf will also think they are real relocations?

Use another early out

craig.topper retitled this revision from [lld][RISCV][WIP] Implement GP relaxation for R_RISCV_HI20/R_RISCV_LO12_I/R_RISCV_LO12_S. to [lld][RISCV] Implement GP relaxation for R_RISCV_HI20/R_RISCV_LO12_I/R_RISCV_LO12_S..Feb 13 2023, 1:35 PM

@MaskRay here's some data I collected on SPEC2006 .text size and dynamic instruction counts with and without GP relaxation using the GNU linker. I also measured using GP as a callee-saved register. https://docs.google.com/spreadsheets/d/14V7cPbyc80AcGHzsMaw9hYb232dzRbGCmTApnxj-SpU/edit?usp=sharing

@MaskRay here's some data I collected on SPEC2006 .text size and dynamic instruction counts with and without GP relaxation using the GNU linker. I also measured using GP as a callee-saved register. https://docs.google.com/spreadsheets/d/14V7cPbyc80AcGHzsMaw9hYb232dzRbGCmTApnxj-SpU/edit?usp=sharing

TL;DR is neither really gets you much of a saving and the only real benefit comes from tiny benchmarks / embedded use cases?..

@MaskRay here's some data I collected on SPEC2006 .text size and dynamic instruction counts with and without GP relaxation using the GNU linker. I also measured using GP as a callee-saved register. https://docs.google.com/spreadsheets/d/14V7cPbyc80AcGHzsMaw9hYb232dzRbGCmTApnxj-SpU/edit?usp=sharing

TL;DR is neither really gets you much of a saving and the only real benefit comes from tiny benchmarks / embedded use cases?..

I think that's a fair summary.

Given that GP is already reserved at least for linux can this patch move forward?

llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
49

@jrtc27 what should I do here? As I said, I think GNU readelf also thinks they are real relocations.

asb added a comment.Mar 1 2023, 11:23 AM

FWIW I'm in favour of support for this, even if it shows very marginal benefit. We're going to get a constant stream of people asking about lack of support or wondering if this feature would improve code size / performance for them.

MaskRay added inline comments.Mar 1 2023, 2:05 PM
lld/ELF/Writer.cpp
855

Make RF_RISCV_NOT_SBSS = RF_PPC_NOT_TOCBSS so that we don't need to shift the values for RF_* above. RISCV and PPC are mutual exclusive.

855

Ignore this comment. I think D145118 renders this part unneeded.

977

The change is not tested. I created D145118 to fix another thing related to __bss_start, so you can drop this change.

lld/docs/ld.lld.1
353

This can be removed. This manpage doesn't document default options.

lld/test/ELF/riscv-relax-hi20-lo12.s
6

%t.rv32c.o and %t.rv64c.o are unused.

31

I usually test at least two symbols so that I can check in-bounds and out-of-bounds in one file. It is useful to test the largest displacement which is still relaxable.

You may check how riscv-relax-call2.s and x86-64-tlsdesc-ld.s test stuff.

Also, like riscv-relax-align.s, we'd better test that symbol values after the relaxed location is still correct.

llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
49

They are ABI-removed relocation types only for GNU ld's internal use. Since these relocation types are not allowed in input files, we can define the constants only in an lld/ELF file.

MaskRay added a subscriber: lazyparser.EditedMar 1 2023, 2:14 PM

Ping

This patch is on my radar. I have been asking OSDT folks (@lazyparser's colleagues) to provide performance and code size analysis. So far the performance analysis says there is only noise (or oddly GP relaxation may negatively affect the performance by 0.x%...). I am still waiting for code size results...

FWIW I'm in favour of support for this, even if it shows very marginal benefit. We're going to get a constant stream of people asking about lack of support or wondering if this feature would improve code size / performance for them.

Honestly I do not know whether we have a constant stream of people... The benefit does seem small, but if the code change can be kept to a minimum, we can still consider adding it.

lld/ELF/Arch/RISCV.cpp
640

remove blank line

lld/ELF/Writer.cpp
1861–1865

We can also set riscvGlobalPointer only if config->relaxGp, then we can remove config->relaxGp test in RISCV.cpp. In RISCV.cpp, we test both config->relaxGp and riscvGlobalPointer. I think it'd be good to reduce the number of tests.

lld/test/ELF/riscv-relax-hi20-lo12.s
7

We may need a test for -pie.

craig.topper added inline comments.Mar 1 2023, 3:43 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
49

Is there an existing file for that? And are there different values I should use?

craig.topper added inline comments.Mar 1 2023, 3:47 PM
lld/test/ELF/riscv-relax-hi20-lo12.s
7

-pie should use %pcrel relocations right?

craig.topper added inline comments.Mar 1 2023, 4:43 PM
lld/test/ELF/riscv-relax-hi20-lo12.s
31

Which part of riscv-relax-align.s tests that?

Address some review comments

MaskRay added inline comments.Mar 1 2023, 5:06 PM
lld/test/ELF/riscv-relax-hi20-lo12.s
7

We should have a meaningful diagnostic referencing a non-absolute symbol with an absolute relocation in -pie mode. It probably doesn't belong to this test.

In addition, it's valid to reference SHN_ABS symbols in -pie mode. We should test that SHN_ABS symbols are correctly handled. Whether it's handled doesn't matter since such cases are super rare.

31

You can just define a symbol in the end of this file and test its value.

craig.topper added inline comments.Mar 1 2023, 5:43 PM
lld/test/ELF/riscv-relax-hi20-lo12.s
7

How do I make an SHN_ABS symbol?

Add symbol to end of test

Joe added a subscriber: Joe.Mar 8 2023, 8:32 AM
Joe added inline comments.
lld/ELF/Writer.cpp
1861

If the global pointer has been defined in the linkerscript, addOptionalRegular will return nullptr and ElfSym::riscvGlobalPointer won't be set. Is it worth adding a check for this and setting it if the symbol is defined?

Joe added inline comments.Mar 8 2023, 9:23 AM
lld/ELF/Arch/RISCV.cpp
443

val is sign extended here, but gp VA is not. This would cause errors on 32 bit if the MSB of gp VA is set

MaskRay added inline comments.Mar 8 2023, 10:58 AM
lld/test/ELF/riscv-relax-hi20-lo12.s
7

Something like

.globl abs
abs = 0x10000
abs_local = 0x1000
llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
49

No existing file use internal relocation type this way. But you can just define two constants in lld/ELF/Arch/RISCV.cpp. (R_RISCV_* will be poor names since they aren't official relocation type names.

Try to address review comments.

This patch seems mostly ready and should make D147983 unneeded. See D147983 that some users are waiting as well.

lld/ELF/Arch/RISCV.cpp
53

Don't use the binutils numbers which may conflict with future relocation types.

For now, just use numbers starting with 256, which are unavailable for ELFCLASS32.

644

Remove this comment or be more specific about which instruction is to be removed. See the case R_RISCV_TPREL_ADD example.

lld/ELF/Options.td
359

It seems that "global pointer relaxation" is the canonical term. Use it instead of "GP relaxation".

lld/ELF/Writer.cpp
1861

See @Joe 's comment.

craig.topper added inline comments.Apr 11 2023, 12:54 PM
lld/ELF/Writer.cpp
1861

I don't know my way around lld's APIs. Can you tell me what I need to do?

craig.topper edited the summary of this revision. (Show Details)Apr 11 2023, 12:54 PM
MaskRay added inline comments.Apr 11 2023, 1:16 PM
lld/ELF/Writer.cpp
1861

Sure. (Sorry I didn't make comments earlier. I had been travelling for most days in the past month and just returned and started to read more patches.)

For library use cases (prevent riscvGlobalPointer from referencing a dangling object), ensure riscvGlobalPointer is always initialized even if config->shared is true.

if (config->emachine == EM_RISCV) {
  ElfSym::riscvGlobalPointer = nullptr;
  if (!config->shared) {
    ...
    ElfSym::riscvGlobalPointer = symtab.find(...);
    addOptionalRegular(...);
  }
}
craig.topper added inline comments.Apr 11 2023, 2:08 PM
lld/ELF/Writer.cpp
1861

Am I still supposed to check config->relaxGP too?

MaskRay added inline comments.Apr 11 2023, 2:51 PM
lld/ELF/Writer.cpp
1861

Yes. ElfSym::riscvGlobalPointer can be nullptr if config->relaxGP is false.

Address review comments

MaskRay added inline comments.Apr 11 2023, 4:40 PM
lld/ELF/Arch/RISCV.cpp
445

uint32_t insn = (read32le(loc) & ~(31 << 15)) | (X_GP << 15)

635

const Defined *gp = ElfSym::riscvGlobalPointer;

lld/ELF/Writer.cpp
1867
lld/test/ELF/riscv-relax-hi20-lo12-pie.s
1 ↗(On Diff #512618)

Merge the tests into riscv-relax-hi20-lo12.s. If the -no-pie test has exercised many cases, the -pie case can just check a few lines if you feel duplicating all -no-pie CHECK lines looks too verbose.

Also, add a -shared test. Note, it's not necessary to test all riscv{32,64} x {-pie,-shared} combinations.
riscv32 -pie and riscv64 -shared may suffice.

craig.topper added inline comments.Apr 11 2023, 7:54 PM
lld/test/ELF/riscv-relax-hi20-lo12-pie.s
1 ↗(On Diff #512618)

I think I got an error for %lo/%hi on the non-absolute symbols with -pie

craig.topper added inline comments.Apr 11 2023, 7:56 PM
lld/test/ELF/riscv-relax-hi20-lo12-pie.s
1 ↗(On Diff #512618)

For example

ld.lld: error: relocation R_RISCV_LO12_S cannot be used against symbol 'norelax'; recompile with -fPIC

Address some review comments

MaskRay accepted this revision.Apr 12 2023, 6:05 PM

Looks great!

This revision is now accepted and ready to land.Apr 12 2023, 6:05 PM
MaskRay added a comment.EditedApr 12 2023, 6:06 PM

I have some concern about whether it is safe to look at symbols in multiple sections during the instruction relaxation phase. Can the relative distance between _global_pointer and .sbss change when the .text section shrinks?

I know there is some question about whether GP should exist at all, so this feature is off by default and must be enabled by passing --relax-gp to the linker.

No tests yet. This is my first time working in lld so need some time to learn. The patch has been tested locally on a statically linked build of SPEC2006 with LTO.

This part in the summary is very outdated. You can remove them also the "No test" part, and reference https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/371

craig.topper edited the summary of this revision. (Show Details)Apr 13 2023, 10:19 AM

I have some concern about whether it is safe to look at symbols in multiple sections during the instruction relaxation phase. Can the relative distance between _global_pointer and .sbss change when the .text section shrinks?

@MaskRay I don't think we ever specifically discussed this. Is this not possible? I think binutils has some check for maximum alignment when relaxing across sections.

craig.topper edited the summary of this revision. (Show Details)Apr 13 2023, 10:25 AM

I have some concern about whether it is safe to look at symbols in multiple sections during the instruction relaxation phase. Can the relative distance between _global_pointer and .sbss change when the .text section shrinks?

@MaskRay I don't think we ever specifically discussed this. Is this not possible? I think binutils has some check for maximum alignment when relaxing across sections.

lld's relaxation approach is safe. The instruction relaxation is done with other transforms that may affect addresses. We iterate until all addresses and symbol values converge.
I think GNU ld has multiple iterations and after the relaxation iteration, post iterations may cause a pathological case for relaxation. It seems to play with heuristics to make things safer. Overall, this is GNU ld's specific issue and unrelated to lld :)

This revision was landed with ongoing or failed builds.Apr 13 2023, 10:52 AM
This revision was automatically updated to reflect the committed changes.