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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp marked 2 inline comments as done.
stefanp 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.
----------------
sfertile wrote:
> Could the `GlobalValue` be a `GlobalindirectSymbol` instead of a function?
Good point. I'll add another case for the `GlobalIndirectSymbol` as well.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4924
   // Check if we share the TOC base.
   if (!Subtarget.isUsingPCRelativeCalls() &&
+      !callDoesNotRequireTOCRestore(&Caller, Callee, getTargetMachine()))
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5285
     return PPCISD::CALL_NOTOC;
   }
 
----------------
Anything PC Relative shouldn't get past this.


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-local-caller-toc.ll:8
+; enabled and both functions are in the same file.
+; Note that the callee does not know if it clobbers the TOC because it
+; contains an external call to @externalFunc.
----------------
sfertile wrote:
> Whats the behavior if we call a function which uses pc-relative instructions to access global data, but has no calls and otherwise preserves the toc-pointer?
If the function uses PC relative instructions for all access to global data and does not have a call then we mark the function with `st_other=0` indicating that it does not clobber the TOC. Unfortunately, we do not have this information early enough to take advantage of it in PPCISelLowering.


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-local-caller-toc.ll:62
+  %add = add nsw i32 %0, %a
+  %call = tail call signext i32 @callee(i32 signext %add)
+  %mul = mul nsw i32 %call, %call
----------------
sfertile wrote:
> If this is instead in tail call position, then the test also documents how we can't tail call from a TOC-based caller to a pc-rel based callee, because we need to restore the toc-pointer after the call.
You are correct. We also cannot tail call because we need to restore the TOC after the call.
This test does not cover that because of the `%mul = mul nsw i32 %call, %call` after the call but I will add a test to cover that situation.


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

https://reviews.llvm.org/D81126