[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:72
+};
+static bool checkPPCInsn(PPCInsn insn) {
+#define PPC_INSN_CHECK
----------------
This helper should return true for any 4-byte load/store instruction that is optimizable as part of a pc-rel access sequence? If so a better name would be `isPCRelOptimizable` or something along those lines.


================
Comment at: lld/ELF/Arch/PPCInsns.def:11
+// Loads.
+PPC_INSN(LBZ, 34)
+PPC_INSN(PLBZ, PREFIX_MLS) // Prefix only.
----------------
This still isn't really the form of a .def file.

1) The enum values are supposed to be defined elsewhere, and the def file is included places where those definitions are visible. 
2) I though we agreed to use the encodings directly as the enum values in our previous discussion. So `LBZ` would be `0x88000000` not `34`.
3) We should have 2 distinct enum types, 1 for the non-prefixed instructions (which use an underlying  4 byte integral type) and the 1 for the prefixed instructions which uses an 8-byte type. 

Then we should include the 4-byte instruction, the prefixed instruction it maps to, and the operand mask in the macro:
PCREL_OPT(Instruction, Prefixed_Equivalent, Operand_Mask)

Ex:
```
PCREL_OPT(LBZ, PLBZ, OPC_AND_RST)
```

And the helpers are implemented by defining the macro. For example to implement `getPCRelativeForm`:
```
switch(arg)
{
default:
  return NOINSN;
#define PCREL_OPT(Encoding, Pcrel_Encoding, Op_mask) case Encoding: return Pcrel_Encoding;
#include PPCInsns.def
#undef PCREL_OPT
}
```

Similarly:
For `checkPPCInsn` 

```
switch(Encoding) 
{
default return false;
 #define PCREL_OPT(Encoding, Pcrel_Encoding, Op_mask) case Encoding: return true;
#include PPCInsns.def
#undef PCREL_OPT
}
```

ditto for `getInsnMask`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360