[PATCH] D81457: [LLD][PowerPC] Add support for R_PPC64_PCREL34

Thu Jun 11 08:47:03 PDT 2020

sfertile added inline comments.

================
Comment at: lld/ELF/Arch/PPC64.cpp:382
+// pieces separately and assemble/disassemble them accordingly.
+static void writePrefixedInstruction(uint8_t *loc, uint64_t insn) {
+  insn = config->isLE ? insn << 32 | insn >> 32 : insn;
----------------
Can I ask the rational on why we want to treat a prefixed instruction as a single 8-byte instruction as opposed to a 4-byte prefix and a 4-byte instruction?  I'm not necessarily disagreeing we should treat them as a single entity, I just want to make sure we have though about what the best representation is considering all the new relocations to be added.

================
Comment at: lld/ELF/Arch/PPC64.cpp:1011
+    uint64_t si1 = (val & si1Mask);
+    uint64_t instr = readPrefixedInstruction(loc) & ~fullMask;
+
----------------
I've got a patch which removes the masking of the instruction bits on the other relocations types we were doing it for [[ https://reviews.llvm.org/D81106 | (D81106)]].  My understanding is that for REL ELF archs the addend is encoded into the bits to be relocated, and for RELA ELF archs the bits to be relocated are zeros, with the addend in the relocation. The extra masking doesn't hurt anything functionally but it can lead to misunderstandings on what the bits to be relocated should  [[ https://reviews.llvm.org/D81082 | contain]] so I think its important to be precise and omit the extra masking despite it not hurting anything. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81457