[PATCH] D114656: [ELF][PPC64] Remove unneeded PPC64PCRelLongBranchThunk

[Victor Huang via Phabricator via llvm-commits]

NeHuang accepted this revision.
NeHuang added a comment.

Thanks @MaskRay for posting this patch! I had similar changes locally but did not have a chance to post it.  : (

Overall the patch LGTM except the name for the thunk as Nemanja suggest. The only test case needs to be changed is `lld/test/ELF/ppc64-pcrel-long-branch.s`  Diff as below FYI.

  @@ -27,7 +27,7 @@
   # CHECK-NEXT:          trap
  
   ## Callee address - program counter = 0x2002000 - 0x2010 = 33554416
   -# CHECK-LABEL: <__long_branch_pcrel_high>:
  +# CHECK-LABEL: <__gep_setup_high>:
   # CHECK-NEXT:    2010: paddi 12, 0, 33554416, 1

You can address the comment while committing the patch.



================
Comment at: lld/ELF/Thunks.cpp:985
 void PPC64R12SetupStub::addSymbols(ThunkSection &isec) {
-  addSymbol(saver.save("__gep_setup_" + destination.getName()), STT_FUNC, 0,
-            isec);
+  addSymbol(saver.save("__long_branch_pcrel_" + destination.getName()),
+            STT_FUNC, 0, isec);
----------------
nemanjai wrote:
> I am really not a fan of the name. The reason we emit this has nothing to do with long branches and the code we emit is not necessarily PC-Relative.
> 
> We emit this thunk when we need to branch to a GEP (Global Entry Point) of local function `Function1` simply because the caller doesn't maintain a TOC pointer and `Function1` does.
> 
> The existing name makes sense because the GEP assumes that `r12` contains the address of the GEP itself (i.e. at the first instruction in the GEP, `r12 == pc`).
+1, the existing name makes more sense which is consistent the functionality of the thunk. Would be better we keep `__gep_setup_` as the name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114656