[PATCH] D84360: [LLD][PowerPC] Implement GOT to PC-Rel relaxation

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

I haven't looked into the functional part of the patch. Just made some style issues and spot a few bugs due to incorrect `-1lu` uses.



================
Comment at: lld/ELF/Arch/PPC64.cpp:69
+// Extracts the 'PO' field of an instruction encoding.
+static uint8_t getPrimaryOpCode(uint32_t encoding) { return (encoding >> 26); }
+
----------------
`return encoding >> 26;`


================
Comment at: lld/ELF/Arch/PPC64.cpp:486
+  case R_PPC64_GOT_PCREL34: {
+    uint64_t insn = readPrefixedInstruction(loc);
+
----------------
Nit: `uint64_t insn = readPrefixedInstruction(loc) & ~0xff000000fc000000;` and move the comment below here


================
Comment at: lld/ELF/Arch/PPC64.cpp:490
+    // instruction (the primary opcode).
+    insn &= ~0xFF000000FC000000lu;
+
----------------
This file is not consistent on upper/lower case hexadecimals but for newer code we use lower case to be consistent with other files. The suffix `lu` is redundant (in any case not proper on 32-bit systems where unsigned long is 32-bit).


================
Comment at: lld/ELF/Arch/PPC64.cpp:509
+    // removed at a later date.
+    if (pcRelInsn == -1lu) {
+      error("unrecognized instruction for R_PPC64_PCREL_OPT relaxation: 0x" +
----------------
-1lu is wrong on a 32-bit platform: 4294967295.

`pcRelInsn == UINT64_C(-1)`


================
Comment at: lld/ELF/Arch/PPC64.cpp:510
+    if (pcRelInsn == -1lu) {
+      error("unrecognized instruction for R_PPC64_PCREL_OPT relaxation: 0x" +
+            Twine::utohexstr(accessInsn));
----------------
Consider using more `errorOrWarn` which can downgrade to warnings if --noinhibit-exec is implemented. `error` is used by code which can't really proceed. `fatal` is fatal error which should call `exit` immediately.


================
Comment at: lld/ELF/Arch/PPC64.cpp:519
+    uint64_t finalInsn = dispOnly | pcRelInsn;
+    writePrefixedInstruction(loc, finalInsn);
+    write32(loc + rel.addend, 0x60000000); // nop accessInsn.
----------------
The two variables are only used once. Just write `(insn & 0x0003ffff0000ffff) | pcRelInsn`


================
Comment at: lld/ELF/InputSection.cpp:1002
   const unsigned bits = config->wordsize * 8;
+  uint64_t lastPPCRelaxedRelocOff = -1lu;
 
----------------
This is wrong on 32-bit platforms.


================
Comment at: lld/test/ELF/Inputs/ppc64-got-to-pcrel-relaxation-def.s:4
+	.comm	useVal_vector,8,8
+	.globl storeVal_longlong
+	.globl useAddr_longlong
----------------
`.globl a, b, c, d` can declare multiple variables


================
Comment at: lld/test/ELF/Inputs/ppc64-got-to-pcrel-relaxation-def.s:31
+	.type	storeVal_longlong, @object
+	.size	storeVal_longlong, 8
+storeVal_longlong:
----------------
Are these `.size` and `.type` directives used? Dropping them can make the file much smaller


================
Comment at: lld/test/ELF/ppc64-got-to-pcrel-relaxation.s:19
+# CHECK-S-LABEL: <check_LBZ_STB>:
+# CHECK-S-NEXT: plbz 10
+# CHECK-S-NEXT: paddi 9
----------------
Some newer lld tests make sure that instructions are indented more than their containing labels
```
# CHECK-S-LABEL: <check_LBZ_STB>:
# CHECK-S-NEXT:    plbz 10
```


================
Comment at: llvm/include/llvm/Target/PPCLegacyToPCRelMap.def:21
+*/
+#if (!defined PPC_LEGACY_TO_PREFIXED_COMPILER) &&                              \
+    (!defined PPC_LEGACY_TO_PREFIXED_LINKER)
----------------
The parentheses are redundant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360