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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai planned changes to this revision.
nemanjai added a comment.

Need to account for a displacement on the access instruction and add some tests along the lines of

  extern int Ext[40];
  int test(int a) {
    return a + Ext[10];
  }



================
Comment at: lld/ELF/Arch/PPC64.cpp:515
+    // accessInsn to a nop.
+    writePrefixedInstruction(loc, (insn & 0x0003ffff0000ffff) | pcRelInsn);
+    write32(loc + rel.addend, 0x60000000); // nop accessInsn.
----------------
As it turns out, the access instruction may actually have a displacement which needs to be added to the computed offset.
I will update this to account for that.


================
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:
----------------
sfertile wrote:
> 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.
I agree with this and will rewrite this portion. This tight coupling between the compiler and the linker is quite unnecessary since there are at least two compilers on the platform and at least 3 linkers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360