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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: lld/ELF/Arch/PPC64.cpp:405
+  if (opc == 61 && (encoding & 0x3) == 0x1) // LXV/STXV.
+    opc |= (encoding & 0x7) << 29;
+
----------------
nemanjai wrote:
> sfertile wrote:
> > Instead of shifting the primary opcode down to the least significant bits, and then shifting the secondary opcodes into the more significant bits, why don't we just use the significant opcode bits as the enum value. 
> > 
> > For example: LDU  0xE8000001, LD = E8000000, LDU = 7C000000. It won't simplify this function since you still need the primary opcode to know how to mask out the encoding bits, but we could follow this patch with an NFC patch where we convert all the other uses of 
> > DFormOpcd and XFormOpcode enums to use PPCInsn and get rid of the other 2 enums. If we do take this root I would suggest having separate enums for the 32-bit instructions and the prefixed instructions.
> While I agree that it is kind of odd to essentially reverse the positions of the primary and extended opcodes, I don't really follow what the details of your proposed update are.
> 
> How in your proposal does `LDU` go from `0xe8000001` to `0x7c000000`?
> I think that any proposal that puts the primary and secondary opcode in the same field is problematic.
> 
> How about keeping the bits where they are for DS and DQ form instructions that have extended opcodes. So lets take primary opcode 58 (`LD, LDU, LWA`):
> ```
> LD  = 0xe8000000
> LDU = 0xe8000001
> LWA = 0xe8000002
> ```
> Then in this function, all DS-Form (PO in { 57, 58, 61, 62 }) would be converted to a `PPCInsn` simply as `encoding & 0xfc000003`.
> 
> For the two versions of DQ-Form, we would have:
> PO == 56 (only LQ) compute as `encoding & 0xfc000000`
> PO == 61 compute as `encoding & 0xfc000007`
> PO == 6 compute as `encoding & 0xfc00000f`
> 
> How does that sound?
Sorry, I should have proofread my comment. The third one was suppsoed to be `LDUX` instead of `LDU again.

Your suggestion sounds perfect.


================
Comment at: lld/ELF/Arch/PPC64.cpp:535
+// For example:
+// pla 3, 1000
+// lwz 3, 20(3)
----------------
nemanjai wrote:
> sfertile wrote:
> > minor nit: The prefixed instruction in the sequence we are going to optimize should always be a paddi, since we relaxed the instruction referenced by the `R_PPC64_GOT_PCREL34` from pld to paddi. My understanding could be wrong though, I expect we can only see the optimization relocation on a got-addressing sequence or a masked load/store sequence (which we don't support yet, but would also start with paddi).
> Sorry, I should probably not use an extended mnemonic here.
> The `pla 3, 1000` is just `paddi 3, 0, 1000, 1`
Thanks, I'm not that familiar with the new prefixed instructions yet so I didn't realize that was a paddi.


================
Comment at: lld/ELF/Relocations.h:99
   R_PPC64_TOCBASE,
+  R_PPC64_RELAX_GOT_PCREL34,
   R_RISCV_ADD,
----------------
nemanjai wrote:
> nemanjai wrote:
> > sfertile wrote:
> > > Should we drop the 'REL34' in the Expr? Its not really important how many bits it represents, and I think the REL is implied by 'PC' in `R_RELAX_GOT_PC`.
> > > 
> > > Also, should it be added to `isRelExpr` in `Relocations.cpp`?
> > > Should we drop the 'REL34' in the Expr? Its not really important how many bits it represents, and I think the REL is implied by 'PC' in `R_RELAX_GOT_PC`.
> > I am definitely OK with this.
> > > 
> > > Also, should it be added to `isRelExpr` in `Relocations.cpp`?
> > This is just an omission on my part as I didn't know that existed. I'll add it.
> > 
> Just to be clear, the name you are suggesting is `R_PPC64_RELAX_GOT_PC`, correct? I don't imagine you would like me to drop `PPC64` since this is specific to this target.
Yep, thats perfect. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360