[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:533
+// paddi 3, 0, 1000, 1
+// lwz 3, 20(3)
+// Should add up to 1020 for total displacement.
----------------
stefanp wrote:
> Is it possible to have:
> ```
>  paddi 3, 0, 1000, 1
>  plwz 3, 20(2)
> ```
> Where both instructions are prefixed.
> I assume probably not (clang won't generate it?) but I figured it is worth asking.
At this point, it should not be possible (plus it would be a bad thing if it happened since we'd only look at a part of the instruction and part of the displacement).
We bail out before the call to this if we don't have a prefixed version of the instruction (which you already observed below).


================
Comment at: lld/ELF/Arch/PPC64.cpp:682
+          "unrecognized instruction for R_PPC64_PCREL_OPT relaxation: 0x" +
+          Twine::utohexstr(accessInsn));
+      break;
----------------
stefanp wrote:
> I guess if the access instruction is already a prefixed instruction there won't be an equivalent PC Relative form of it and we will produce an error here.
Right, if the access instruction is already prefixed, we'd be looking at the prefix and not the instruction. 


================
Comment at: lld/ELF/Arch/PPC64.cpp:620
+    if ((insn & 0xfc000000) != 0xe4000000)
+      error ("expected a 'pld' for got-indirect to pc-relative relaxing");
+    insn &= ~0xff000000fc000000;
----------------
NeHuang wrote:
> nit: clang-format, this can be resolved before committing the changes. 
Not sure what you mean, here. I ran `clang-format` on the patch and the pre-commit check didn't complain about the formatting.


================
Comment at: lld/ELF/Arch/PPC64.cpp:1272
+    assert(data && "Expecting an instruction encoding here");
+    if ((readPrefixedInstruction(data) & 0xfc000000) == 0xe4000000)
+      return R_PPC64_RELAX_GOT_PC;
----------------
NeHuang wrote:
> I have two questions regarding `void PPC64::relaxGot(uint8_t *loc, const Relocation &rel, uint64_t val)` in `lld/ELF/Arch/PCC64cpp` line 619, where had similar check added as below:
> ```
> case R_PPC64_GOT_PCREL34: {
>     // Clear the first 8 bits of the prefix and the first 6 bits of the
> ...
>     if ((insn & 0xfc000000) != 0xe4000000)
>       error ("expected a 'pld' for got-indirect to pc-relative relaxing");
> ...
> ```
> 
> 1. Did it catch the case that `paddi` instruction with `R_PPC64_GOT_PCREL34` emitted from compiler side when it tried to optimize it?
> 2. After adding the new check here, does it make sense to remove the same check in `PPC64::relaxGot`?
> 
1. This check was initially missing so we didn't know that we were mis-optimizing this.
2. I suppose it could be removed, but it is there defensively because if we ever get something other than a `pld` there, it is guaranteed that the resulting executable will be broken. I chose to go with an `error` there rather than an `assert` because it is crucial to fail in such a situation even if we build without asserts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84360