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

Nemanja Ivanovic via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 16 05:03:25 PDT 2020


nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

None of the comments on their own really require another round of review, but there are a number of them so I'd prefer to see the updates before approving them.



================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1690
 
   // Create branch instruction for pseudo tail call return instruction
   unsigned RetOpcode = MBBI->getOpcode();
----------------
Maybe some additional info in this comment along the lines of:
```
// The TCRETURNdi variants are direct calls. Valid targets for those are
// MO_GlobalAddress operands as well as MO_ExternalSymbol with PC-Rel
// since we can tail call external functions with PC-Rel (i.e. don't need to
// worry about different TOC pointers).
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5511
+    // constraints.
+    if (!CFlags.IsIndirect || !Subtarget.isUsingPCRelativeCalls()) {
+      assert(((Callee.getOpcode() == ISD::Register &&
----------------
With non-assert builds, this will be an empty block so we should write this differently. Also, the second assert seems perfectly valid in both cases - won't the opcode still be `PPCISD::TC_RETURN` even for indirect tail calls?
Perhaps we should just add another disjunction to the assert:
```
      assert(((Callee.getOpcode() == ISD::Register &&
               cast<RegisterSDNode>(Callee)->getReg() == PPC::CTR) ||
              Callee.getOpcode() == ISD::TargetExternalSymbol ||
              Callee.getOpcode() == ISD::TargetGlobalAddress ||
              isa<ConstantSDNode>(Callee) ||
              (CFlags.IsIndirect && Subtarget.isUsingPCRelativeCalls()) &&
             "Expecting a global address, external symbol, absolute value, "
             "register or an indirect call (with PC-Rel addressing)");
```


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


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


================
Comment at: llvm/lib/Target/PowerPC/PPCMCInstLower.cpp:102
+  } else {
+    assert(MIOpcode != PPC::BL8_NOTOC &&
+           "BL8_NOTOC is only valid when using PC Relative Calls.");
----------------
Please don't create conditional block that contain only asserts.
This assert can be placed before this code and just be
```
assert((Subtarget->isUsingPCRelativeCalls() ||
        MIOpcode != PPC::BL8_NOTOC) &&
        "BL8_NOTOC is only valid when using PC Relative Calls.");
```


================
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
----------------
Why are we adding `-enable-ppc-quad-precision`? I don't see any use of float128.


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

https://reviews.llvm.org/D77788





More information about the llvm-commits mailing list