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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Overall I think this looks good. I had a handful of comments but nothing major.
I wish we had a better way of doing the conversion between the old forms and the PCRel forms but I can't think of a better way.



================
Comment at: lld/ELF/Arch/PPC64.cpp:42
 
 enum DFormOpcd {
+  /* Defined below in the PPC enum so they can be used for mapping legacy
----------------
nemanjai wrote:
> Perhaps this entire `enum` should just be absorbed into the new one to eliminate confusion.
My two cents:
I would agree except that the file `PPCLegacyToPCRelMap.def` is meant specifically for PCRel (as is in the title of the file) and the update forms have nothing to do with that really... 

We should clean these up but not with the PC Rel instructions.


================
Comment at: llvm/include/llvm/Target/PPCLegacyToPCRelMap.def:72
+  DFLOADf32 = 48,
+  DFLOADf64 = 50,
+
----------------
nit:
You should probably add
```
DFLOADf32 = 48,  //  lfs
DFLOADf64 = 50,  //  lfd
```
The names DFLOADf32 and DFLOADf64 are internal names we have that won't mean anything to anyone that is not familiar with the compiler. The `lfs` and `lfd` are in the ISA.


================
Comment at: llvm/include/llvm/Target/PPCLegacyToPCRelMap.def:196
+  return getInstrMapIdx(Opc) != -1u;
+}
+
----------------
I would almost prefer to have this function in the compiler in `PPCPreEmitPeephole.cpp`.
The function was in the compiler in the first place and it is only used in that one file.

If we do it that way we can get rid of the macro `PPC_LEGACY_TO_PREFIXED_COMPILER`.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:50
+
+class PPCPreEmitPeephole : public MachineFunctionPass {
+public:
----------------
nit:
I get the feeling that clang-format went a little crazy on this file and removed the indentation. :)
Should probably be a separate NFC patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360