[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:72
+};
+static bool checkPPCInsn(PPCInsn insn) {
+#define PPC_INSN_CHECK
----------------
sfertile wrote:
> 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.
Not really. This is meant to check whether the underlying value is one of the enumerators or some value outside of the enumeration.
For example:
`checkPPCinsn(static_cast<PPCInsn>(0x6100000A4000000) == true` since it is `PLWA`
while
`checkPPCinsn(static_cast<PPCInsn>(0x6100000A400ABCD) == false` because it is not the value of any enumerator.


================
Comment at: lld/ELF/Arch/PPCInsns.def:11
+// Loads.
+PPC_INSN(LBZ, 34)
+PPC_INSN(PLBZ, PREFIX_MLS) // Prefix only.
----------------
sfertile wrote:
> 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`
1. I don't think this is true. There are plenty of examples where the enumerators are specified in a .def file. For example `include/llvm/IR/Instruction.def`. I prefer to leave the defs in the .def file.
2. This is just a bit of miscommunication. I thought we were only talking about instructions for which we need the extended opcode to disambiguate.
3. I am not sure how this improves anything, but sure - since I am indifferent and you have a preference, I'll go with your preference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360