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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/include/llvm/Target/PPCLegacyToPCRelMap.def:72
+  DFLOADf32 = 48,
+  DFLOADf64 = 50,
+
----------------
stefanp wrote:
> 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.
For sure. Will do.


================
Comment at: llvm/include/llvm/Target/PPCLegacyToPCRelMap.def:196
+  return getInstrMapIdx(Opc) != -1u;
+}
+
----------------
stefanp wrote:
> 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`.
I don't think it would be a good thing to get rid of that macro. I have added the `#error` in this file to require that one of the two be defined so that this file cannot be included accidentally somewhere else and cause namespace pollution.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:50
+
+class PPCPreEmitPeephole : public MachineFunctionPass {
+public:
----------------
stefanp wrote:
> 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.
Yeah, I have no idea what `clang-format`'s problem is - why is it modifying lines that I never touched in this patch? I don't mind pre-committing an NFC patch from running `clang-format` on this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360