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

[Rafael Auler via Phabricator via llvm-commits]

rafauler 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())
----------------
jobnoorman wrote:
> 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?
I see, it makes sense. You're correct in your assessment and it should be harmless. it does increase the chance that BOLT might incorrectly update some value that didn't even need updating in the first place, although that would be a hidden bug in BOLT that would need to be solved anyway.


================
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)
----------------
jobnoorman wrote:
> 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).
Thanks for explaining


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146554