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

Thu Jul 30 16:03:18 PDT 2020

nemanjai added inline comments.

================
Comment at: lld/ELF/Arch/PPC64.cpp:490
+    // instruction (the primary opcode).
+    insn &= ~0xFF000000FC000000lu;
+
----------------
MaskRay wrote:
> 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).
I can correct all the inconsistencies in an NFC patch after this lands. I will make sure I don't add any upper case ones.

================
Comment at: lld/test/ELF/ppc64-got-to-pcrel-relaxation.s:168
+check_LWA_STW:
+  mr 9,3
+  pld 8,useVal_sint at got@pcrel(0),1
----------------
MaskRay wrote:
> Can these unrelated instructions be removed? We just need prefixed instructions and for the `.reloc` case, one instruction before `.reloc`
I could remove them. I am reluctant to do that because the asm doesn't really make sense if we remove them. Although I realize this doesn't make a difference when linking, I just find it less visually jarring to have asm that is clearly incorrect (i.e. storing an undefined register in this case). And removing them doesn't save that many lines.

Of course, if you insist, I will remove them.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360