[PATCH] D81948: [LLD][PowerPC] Add support for R_PPC64_GOT_PCREL34

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a subscriber: grimar.
MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/PPC64.cpp:1019
+  case R_PPC64_GOT_PCREL34: {
+    uint64_t si0Mask = 0x00000003FFFF0000;
+    uint64_t si1Mask = 0x000000000000FFFF;
----------------
sfertile wrote:
> @MaskRay comments on the other patch about lowercase hex literals applies here as well. Ditto for the suggested test change (ie labels instead of sections)
In https://reviews.llvm.org/D81986#inline-753874 @grimar asked whether we should use `const`. It seems to me that is a good idea. Many other components of LLVM use `const` whenever appropriate.


================
Comment at: lld/test/ELF/ppc64-got-pcrel34-reloc.s:1
+# REQUIRES: ppc
+
----------------
The naming is `ppc64-reloc-*` (x86-64-reloc- tests stick with this rule more consistently)


================
Comment at: lld/test/ELF/ppc64-got-pcrel34-reloc.s:18
+_start:
+# CHECK-LABEL: Disassembly of section .GLOB_INT_PCREL
+# CHECK:       10340:        pld 3, 565808(0), 1
----------------
Consider using a label instead of a `.section` as the anchor in the output. This allows you to use `-NEXT:` which makes tests easier to update when something goes off.

```
# CHECK:      <foo>:
# CHECK-NEXT:   pld 3, ...   # I tend to indent instructions to make it clear the region is covered by the previous label
```

Omit unneeded addresses unless you do some arithmetic and somewhere else in the test requires exact addresses. In that case, you should write the formula in a comment (example: ppc64-tls-*.s)


================
Comment at: lld/test/ELF/ppc64-got-pcrel34-reloc.s:33
+# RELA:        0009a578  0000000200000014 R_PPC64_GLOB_DAT       0000000000000000 glob_int8 + 0
+.section .GLOB_INT_PCREL_OFFSET,"ax", at progbits
+	pld 3, glob_int8 at got@PCREL(0), 1
----------------
sfertile wrote:
> It this test just to show the linker doesn't modify the load instruction that follows the PC-relative load from the got?
If you want to check not TOC (a variant of GOT) optimization is performed, use -no-pie (default) or -pie, instead of -shared.


================
Comment at: lld/test/ELF/ppc64-got-pcrel34-reloc.s:44
+# RELA:        0009a580  0000000300000014 R_PPC64_GLOB_DAT       0000000000000000 glob_int8_big + 0
+.space 500000
+.section .GLOB_INT_PCREL_BIGOFFSET,"ax", at progbits
----------------
This creates a large file. Use a linker script like ppc32-long-thunk.s


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81948