[PATCH] D143673: [lld][RISCV] Implement GP relaxation for R_RISCV_HI20/R_RISCV_LO12_I/R_RISCV_LO12_S.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Writer.cpp:842
+  RF_BSS = 1 << 9,
+  RF_RISCV_NOT_SBSS = 1 << 8,
+  RF_RISCV_SDATA = 1 << 7,
----------------
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.


================
Comment at: lld/ELF/Writer.cpp:977
+    if (osec.name != ".sbss")
+      rank |= 2; // TODO: Proper constant
+    if (osec.name == ".sdata")
----------------
craig.topper wrote:
> @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?
The change is not tested. I created D145118 to fix another thing related to `__bss_start`, so you can drop this change.


================
Comment at: lld/docs/ld.lld.1:353
 Disable target-specific relaxations. For x86-64 this disables R_X86_64_GOTPCRELX and R_X86_64_REX_GOTPCRELX GOT optimization.
+.It Fl --no-relax-gp
+Disable GP relaxation for RISC-V.
----------------
This can be removed. This manpage doesn't document default options.


================
Comment at: lld/test/ELF/riscv-relax-hi20-lo12.s:5
+# RUN: llvm-mc -filetype=obj -triple=riscv64-unknown-elf -mattr=+relax %s -o %t.rv64.o
+# RUN: llvm-mc -filetype=obj -triple=riscv32-unknown-elf -mattr=+c,+relax %s -o %t.rv32c.o
+# RUN: llvm-mc -filetype=obj -triple=riscv64-unknown-elf -mattr=+c,+relax %s -o %t.rv64c.o
----------------
%t.rv32c.o and %t.rv64c.o are unused.


================
Comment at: lld/test/ELF/riscv-relax-hi20-lo12.s:30
+_start:
+  lui a0, %hi(foo)
+  addi a0, a0, %lo(foo)
----------------
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.


================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def:49
 ELF_RELOC(R_RISCV_RVC_LUI,           46)
+ELF_RELOC(R_RISCV_GPREL_I,           47)
+ELF_RELOC(R_RISCV_GPREL_S,           48)
----------------
craig.topper wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > jrtc27 wrote:
> > > > craig.topper wrote:
> > > > > jrtc27 wrote:
> > > > > > These don't exist, they were removed as they were solely internal to binutils, and their encodings are reserved for future standard use
> > > > > 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?
> > > > 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.
> > > 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.
> > 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?
> @jrtc27 what should I do here? As I said, I think GNU readelf also thinks they are real relocations.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143673/new/

https://reviews.llvm.org/D143673