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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lld/ELF/Arch/PPC64.cpp:405
+  if (opc == 61 && (encoding & 0x3) == 0x1) // LXV/STXV.
+    opc |= (encoding & 0x7) << 29;
+
----------------
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?


================
Comment at: lld/ELF/Arch/PPC64.cpp:535
+// For example:
+// pla 3, 1000
+// lwz 3, 20(3)
----------------
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`


================
Comment at: lld/ELF/Relocations.h:99
   R_PPC64_TOCBASE,
+  R_PPC64_RELAX_GOT_PCREL34,
   R_RISCV_ADD,
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360