[PATCH] D77788: [PowerPC][Future] Enable Tail Calls for PC Relative Code

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

Thanks for addressing my comments. The remaining nits do not at all seem like a reason to block the approval. LGTM.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4796
+    // No TCO/SCO on indirect call because Caller have to restore its TOC
+    if (!isFunctionGlobalAddress(Callee) && !isa<ExternalSymbolSDNode>(Callee))
+      return false;
----------------
stefanp wrote:
> nemanjai wrote:
> > It is interesting that we have `!isa<ExternalSymbolSDNode>(Callee)` when the subsequent check will immediately return false if `!isFunctionGlobalAddress(Callee)`.
> > I think we should simplify this into:
> > ```
> > // All variants of 64-bit ELF ABIs without PC-Relative addressing
> > // require that the caller and callee share the same TOC for
> > // TCO/SCO. We cannot guarantee this for indirect
> > // calls or calls to external functions. When PC-Relative addressing
> > // is used, the concept of the TOC is no longer applicable so this
> > // check is not required.
> > if (!Subtarget.isUsingPCRelativeCalls() &&
> >     !callsShareTOCBase(&Caller, Callee, getTargetMachine()))
> >   return false;
> > ```
> Actually we do need both guards. When I first saw your comment I thought the same thing but then I realized (after `ppc64-nonfunc-calls.ll` failed) that `isFunctionGlobalAddress` is not the same as `GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee);` which is tested as the first thing in `callsShareTOCBase`.
> Here is that check. This function can still return false even if `Callee` is a `GlobalAddressSDNode`.
> ```
> static bool isFunctionGlobalAddress(SDValue Callee) {
>   if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee)) {
>     if (Callee.getOpcode() == ISD::GlobalTLSAddress ||
>         Callee.getOpcode() == ISD::TargetGlobalTLSAddress)
>       return false;
> 
>     return G->getGlobal()->getValueType()->isFunctionTy();
>   }
> 
>   return false;
> }
> ```
> 
> Take the case where we have a `GlobalSDNode` where the value type is not a function type. (This would be the case for indirect calls.)  The `isFunctionGlobalAddress` would return false but `dyn_cast<GlobalAddressSDNode>(Callee);` would not. 
> I've kept both if conditions but I've flattened them so that `Subtarget.isUsingPCRelativeCalls()` is in both.
Oh, I see. That's great, thanks for the explanation.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5587
              "Callee should be an llvm::Function object.");
-      LLVM_DEBUG(
-          const GlobalValue *GV =
-              cast<GlobalAddressSDNode>(Callee)->getGlobal();
-          const unsigned Width =
-              80 - strlen("TCO caller: ") - strlen(", callee linkage: 0, 0");
-          dbgs() << "TCO caller: "
-                 << left_justify(DAG.getMachineFunction().getName(), Width)
-                 << ", callee linkage: " << GV->getVisibility() << ", "
-                 << GV->getLinkage() << "\n");
+#ifndef NDEBUG
+      if (isa<GlobalAddressSDNode>(Callee)) {
----------------
stefanp wrote:
> nemanjai wrote:
> > I think it is time to refactor this debug dump to something more meaningful. We no longer check the linkage of the callee for deciding to tail call so it seems quite meaningless to show it in the debug dump.
> > I think we can simplify this to something like:
> > ```
> >       LLVM_DEBUG(dbgs() << "TCO caller: " << DAG.getMachineFunction().getName()
> >                         << "\nTCO callee: ");
> >       LLVM_DEBUG(Callee.dump());
> > ```
> > And then we don't need to wrap this in an `NDEBUG` macro.
> FYI:
> Yes, these dubug messages are more useful than what we had before.
> However, the `Callee.dump()` won't always give the user useful information. For indirect calls the Callee node is sometimes a copy like this one:
> C Code:
> ```
> typedef int (*ptrfunc) ();
> int TailCallParamFuncPtr(ptrfunc passedfunc) {
>   return (*passedfunc) ();
> }
> ```
> Produces this debug output:
> ```
> TCO caller: TailCallParamFuncPtr
> TCO callee: t2: i64,ch = CopyFromReg t0, Register:i64 %0
> 
> ```
> Not super useful but much better than the nothing we would output before. :)
> Also, since it's an indirect call we have no way of knowing the name of the function. This is probably the best we can do.
I think that's fairly useful. It really is a value that comes in from outside the function and we don't know anything more about it. The best we can do seems adequate to me.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5522
            "Expecting a global address, external symbol, absolute value or "
            "register");
+    // PC Relative calls also use TC_RETURN as the way to mark tail calls.
----------------
Nit: the message for the assert didn't change according to my comments.


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

https://reviews.llvm.org/D77788