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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp marked 4 inline comments as done.
stefanp added a comment.

Replied to comments. I'll update the patch in a couple of min.



================
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;
----------------
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.


================
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)) {
----------------
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.


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-tail-calls.ll:3
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:   -mcpu=future -enable-ppc-quad-precision -ppc-asm-full-reg-names \
+; RUN:   -ppc-vsr-nums-as-vr < %s | FileCheck %s
----------------
nemanjai wrote:
> Why are we adding `-enable-ppc-quad-precision`? I don't see any use of float128.
Because I copy-pasted these lines form another test? Sorry, I'll fix it. 


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

https://reviews.llvm.org/D77788