[PATCH] D81126: [PowerPC] Fix for PC Relative call protocol

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4721
+
+    const Function *F = dyn_cast<Function>(GV);
+    // Without a valid function pointer we cannot check if the TOC is the same.
----------------
stefanp wrote:
> sfertile wrote:
> > Could the `GlobalValue` be a `GlobalindirectSymbol` instead of a function?
> Good point. I'll add another case for the `GlobalIndirectSymbol` as well.
Hmm, I think we want to try to look through GlobalAliases. Since they must alias a local definition we know the callee, can get it and determine if it uses PCRel or not. If the GlobalIndirectSymbol is an IFunc then the callee is determined at  run-time by the dynamic linker calling the resolver function and I don't think we can determine any of the properties of the callee at compile time.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4924
   // Check if we share the TOC base.
   if (!Subtarget.isUsingPCRelativeCalls() &&
+      !callDoesNotRequireTOCRestore(&Caller, Callee, getTargetMachine()))
----------------
stefanp wrote:
> sfertile wrote:
> > There feels like a disconnect: At one call site we only call the helper function if the subtarget is not using pc-relative calls, but at the other call site we call the helper indiscriminately and rely on logic in  the helper to check if the caller is using pc-relative calls. I think its best to keep the helper as `callsShareTOCBase` and only call it in the case the caller is not using pc-relative calls.  I am assuming that we consider a function using pc-relative calls as not maintaining the toc-pointer, and so the answer in that case is trivially `no, they do not share a toc base regardless of any other properties`.
> Actually, we cannot call `callsShareTOCBase` with PC Relative addressing in either case.
> In this case there is a guard just before the call but in `getCallOpcode` the PC Relative case is handled earlier in the function.
> 
> However, I'll add an assert in `callsShareTOCBase` to make sure that we never reach it when we have PC Relative accesses.
I had missed that thank you. I think the assert you added is a good idea.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4701
+  // is PC Relative since PC Relative callers do not have a TOC.
+  const PPCSubtarget *STICaller = &TM.getSubtarget<PPCSubtarget>(*Caller);
+  assert(!STICaller->isUsingPCRelativeCalls() &&
----------------
If this is only used in the assert then it will causes a warning to be introduced in builds where asserts are disabled.


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

https://reviews.llvm.org/D81126