[PATCH] D146554: [BOLT][RISCV] Implement R_RISCV_ADD32/SUB32

[Job Noorman via Phabricator via llvm-commits]

jobnoorman added inline comments.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2906
+      (opts::ForceToDataRelocations && checkMaxDataRelocations()) ||
+      // RISC-V has ADD/SUB data-to-data relocations
+      BC->isRISCV())
----------------
rafauler wrote:
> Aren't these data-to-code relocs? (BOLT doesn't need to update data-to-data relocs).
> Aren't these data-to-code relocs?

The relevant relocations in the `reloc-jt.s` test case look like this:

```
Relocation section '.rela.data' at offset 0x2068 contains 2 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000002000  000300000023 R_RISCV_ADD32     000000000000100e .LBB0_1 + 0
000000002000  000200000027 R_RISCV_SUB32     0000000000002000 .LJTI0_0 + 0
```

`.LJTI0_0` is a label in `.data` so the `R_RISCV_SUB32` is a data-to-data relocation.

> BOLT doesn't need to update data-to-data relocs

iiuc, BOLT //usually// doesn't need to update data-to-data relocs because the `.data` section is not relocated, correct?

In this case, however, the data-to-data reloc gets composed (see D146546) with a data-to-code reloc (`R_RISCV_ADD32`) so BOLT needs to process it to ensure the linker can properly update the value.

The condition here might be a bit coarse-grained, though, as //all// data-to-data relocs on RISC-V will now be processed even though it might not always be necessary. I suppose this doesn't really matter as the linker will just overwrite data contents with its original value. Or am I missing something here?


================
Comment at: bolt/test/RISCV/reloc-jt.s:13-14
+.LBB0_0:
+  auipc a1, %pcrel_hi(.LJTI0_0)
+  addi a1, a1, %pcrel_lo(.LBB0_0)
+  lw a0, (a1)
----------------
rafauler wrote:
> I'm not sure I get that. Aren't the two relocs supposed to point to the same symbol (.LJTI0_0)?
> Aren't the two relocs supposed to point to the same symbol (.LJTI0_0)?

No, they aren't, but this is something that also used to confuse me :)

Creating a 32-bit PC-relative address on RISC-V works as follows:

1. `auipc` ("add upper immediate to PC") with a `R_RISCV_PCREL_HI20` reloc pointing to the target symbol (`.LJTI0_0` in this case). The result only has the upper 20-bits filled, the lower 12 are set to 0.
2. `addi` (or another instruction with a 12-bit immediate, like `lw`) with a `R_RISCV_PCREL_LO12_I` pointing to the corresponding `auipc` (`.LBB0_0` in this case) to fill in the lower 12-bits.

The psABI has a [good explanation](https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#pc-relative-symbol-addresses) for why it works this way (an important reason is that the `auipc` and `addi` are not necessarily consecutive).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146554