[PATCH] D83504: [PowerPC] Implement R_PPC64_REL24_NOTOC local calls. callee has a TOC

[Victor Huang via Phabricator via llvm-commits]

NeHuang marked 9 inline comments as done.
NeHuang added inline comments.


================
Comment at: lld/ELF/Thunks.cpp:838
+void PPC64R12SetupStub::writeTo(uint8_t *buf) {
+  int64_t offset = destination.getVA() - getThunkTargetSym()->getVA();
+  uint64_t prefix = 0x04100000 | ((offset >> 16) & 0x3ffff);
----------------
sfertile wrote:
> fatal error if the offset is to large to encode in the instruction.
Good point. Added fatal error checking if the offset fits in 34 bits. 


================
Comment at: lld/ELF/Thunks.cpp:842
+
+  writePrefixedInstruction(buf + 0, (prefix << 32) | suffix); // pld r12, func at plt@pcrel
+  write32(buf + 8, 0x7d8903a6);                               // mtctr r12
----------------
sfertile wrote:
> sfertile wrote:
> > Comment is wrong. `destination.getVA() - getThunkTargetSym()->getVA()` is the pc-relative offset to the function (`func at pcrel`), not the offset to the functions plt entry. 
> Sorry, I missed that the comment also says the instruction is a `pld` which would be correct if we are loading the symbols address from the plt, but this thunk type is for functions **not** in the plt. Do you intend to load the address of the function out of a table? or do you meant to calculate the address relative to the program counter?
Good catch! Updated it accordingly. 


================
Comment at: lld/ELF/Thunks.cpp:981
 
+  if (type == R_PPC64_REL24_NOTOC && (s.stOther >> 5) > 1)
+    return make<PPC64R12SetupStub>(s);
----------------
MaskRay wrote:
> What if `(s.stOther >> 5) == 1`?
For (s.stOther >> 5)  < 2 cases (0, 1), they do not need r12 setup stub but could need a long branch thunk. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83504