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

Tue Aug 4 06:51:11 PDT 2020

sfertile added inline comments.

================
Comment at: llvm/include/llvm/Target/PPCLegacyToPCRelMap.def:1
+/* This file defines a mapping between legacy instructions and their
+   PC-relative prefixed versions. It has two uses:
----------------
This file does //way// more then I expect from a `.def` file.  Are there other examples of a .def file defining so much data and code? If there isn't this needs to be moved to a header and potentially a source file since we don't want to change what a .def file represents in LLVM. If there is I still think we are better moving this to a header + source if it stay close to its current implementation using maps.

Alternatively if we split this up back to LLVM and LLD implementations it will make a clean .def fiie in lld.

My suggestion is  to keep the switch in `PPCPreEmitPeephole.cpp` the way it is. I understand wanting to not have more then 1 place to modify when adding a new relaxable instruction is really desirable, but you have an error in LLD if we find the relocation on an unrecognized instruction to catch the mismatches. Combining both llc and lld implementations makes this much more complicated then it needs to be (and like I mentioned previously, inappropriate foe a .def file).

The LLD implementation I think we could turn into a proper .def file by working with the full encodings instead of primary opcodes and using a macro along the lines of:

`MACRO(NONPCREL_ENCODING, PCREL_EQUIVALENT, OPCODE_MASK)`

Then we define the enums in Arch/PPC64.cpp, and have 2 functions with switches over the NONPCREL_ENCODINGS: 1 that maps an encoding to its pc-relative equivalent encoding and one to map it to the operand mask. Instead of mixing in the bits for the non-primary opcode we just zero out the non-opcode bits, then use the 2 helper functions to implement ` getPCRelativeForm` almost exactly the same as it is now.

As a bonus we can re-implement `isInstructionUpdateForm` as a macro in the .def file as well for consistency.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360