[PowerPC] Fix logic dealing with nop after calls (and tail-call eligibility)

[hfinkel]

Summary

This patch aims to unify and correct our logic for when we need to allow for the possibility of the linker adding a TOC restoration instruction after a call. This comes up in two contexts:

1. When determining tail-call eligibility. If we make a tail call (i.e. directly branch to a function) then there is no place for the linker to add a TOC restoration.
2. When determining when we need to add a cop instruction after a call. Likewise, if there is a possibility that the linker might need to add a TOC restoration after a call, then we need to put a nop after the call (the bl instruction).

First problem: We were using similar, but different, logic to decide (1) and (2). This is just wrong. Both the resideInSameModule function (used when determining tail-call eligibility) and the isLocalCall function (used when deciding if the post-call nop is needed) were supposed to be determining the same underlying fact (i.e. might a TOC restoration be needed after the call). The same logic should be used in both places.

Second problem: The logic in both places was wrong. We only know that two functions will share the same TOC when both functions come from the same section of the same object. Otherwise the linker might cause the functions to use different TOC base addresses (unless the multi-TOC linker option is disabled, in which case only shared-library boundaries are relevant). There are a number of factors that can cause functions to be placed in different sections or some from different objects (-ffunction-sections, explicitly-specified section names, COMDAT, weak linkage, etc.). All of these need to be checked. The existing logic only checked properties of the callee, but the properties of the caller must also be checked (for example, calling from a function in a COMDAT section means calling between sections).

There was a conceptual error in the resideInSameModule function in that it allowed tail calls to functions with weak linkage and protected/hidden visibility. While protected/hidden visibility does prevent the function implementation from being replaced at runtime (via interposition), it does not prevent the linker from using an alternate implementation at link time (i.e. using some strong definition to replace the provided weak one during linking). If this happens, then we're still potentially looking at a required TOC restoration upon return.

Otherwise, in general, the post-call nop is needed wherever ELF interposition needs to be supported. We don't currently support ELF interposition at the IR level (see http://lists.llvm.org/pipermail/llvm-dev/2016-November/107625.html for more information), and don't think we should try to make it appear to work in the backend in spite of that fact. This will yield subtle bugs if interposition is attempted. As a result, regardless of whether we're in PIC mode, we don't assume that we need to add the nop to support the possibility of ELF interposition. However, the necessary check is in place (i.e. calling GV->isInterposable and TM.shouldAssumeDSOLocal) so when we have functions for which interposition is allowed at the IR level, we'll add the nop as necessary. In the mean time, we'll generate more tail calls and fewer nops when compiling position-independent code.

[kbarton]

Aside from some minor comments, this LGTM.

lib/Target/PowerPC/PPCISelLowering.cpp

4128

Probably should update this comment, since we're now checking for a section, not a module.
Also, I'm not sure if the link below is still relevant

test/CodeGen/PowerPC/ppc64-blnop.ll

4

Do we also want to test -relocation-model=pic with -mtriple=powerpc64le-unknown-linux-gnu or will the code path be the same for powerpc64-unknown-linux-gnu?

5

Same question here, but for function-sections

[iteratee]

I found what was causing our tests to fail.

llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp

4017 ↗

(On Diff #81349)

This line has to remain for the code to be correct, but it's overly broad. The correct line is:

if (TM.getRelocationModel() == Reloc::PIC_ &&

  GV->hasDefaultVisibility() &&
  !GV->hasLocalLinkage())
return false;

See PPCSubtarget::classifyGlobalReference in PPCSubtarget.cpp

under PIC, any non-local global is interposable, and so the linker will insert a trampoline for those calls.

I'll verify that this fixes the broken test, but it fixes the testcase I extracted.

[iteratee]

I should clarify the problem.

LLVM / Clang may not support ELF interposition. But the linker just doesn't care. If we emit a symbol that is ExternallyAvailable and not local linkage, the linker will emit a trampoline that saves r2, even for a tail call.

This is a problem for a tail call as before the jump, it removes its own stack frame (or it lived in the red zone in the case I debugged), so the r2 being saved by the trampoline is not correct.

a -- calls -> b -- tail calls -> c

if a is in a different module from b, then a's saved copy of r2 will be clobbered by the b->c trampoline that the linker generates.

[hfinkel]

In D27231#628498, @iteratee wrote:

I should clarify the problem.

LLVM / Clang may not support ELF interposition. But the linker just doesn't care. If we emit a symbol that is ExternallyAvailable and not local linkage, the linker will emit a trampoline that saves r2, even for a tail call.

This is a problem for a tail call as before the jump, it removes its own stack frame (or it lived in the red zone in the case I debugged), so the r2 being saved by the trampoline is not correct.

a -- calls -> b -- tail calls -> c

if a is in a different module from b, then a's saved copy of r2 will be clobbered by the b->c trampoline that the linker generates.

Thanks for tracking this down! I'll update the patch accordingly.

[hfinkel]

In D27231#635214, @hfinkel wrote:

In D27231#628498, @iteratee wrote:

I should clarify the problem.

LLVM / Clang may not support ELF interposition. But the linker just doesn't care. If we emit a symbol that is ExternallyAvailable and not local linkage, the linker will emit a trampoline that saves r2, even for a tail call.

This is a problem for a tail call as before the jump, it removes its own stack frame (or it lived in the red zone in the case I debugged), so the r2 being saved by the trampoline is not correct.

a -- calls -> b -- tail calls -> c

if a is in a different module from b, then a's saved copy of r2 will be clobbered by the b->c trampoline that the linker generates.

Thanks for tracking this down! I'll update the patch accordingly.

I've reapplied this in r291003 with a comment based on your explanation.

lib/Target/PowerPC/PPCISelLowering.cpp

if (SPDiff < FI->getTailCallSPDelta())
     FI->setTailCallSPDelta(SPDiff);
 
   return SPDiff;
 }
 
 static bool isFunctionGlobalAddress(SDValue Callee);
 
 static bool
-resideInSameModule(SDValue Callee, Reloc::Model RelMod) {
+resideInSameSection(const Function *Caller, SDValue Callee,
+                    const TargetMachine &TM) {
   // If !G, Callee can be an external symbol.
   GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee);
-  if (!G) return false;
+  if (!G)
+    return false;
 
   const GlobalValue *GV = G->getGlobal();
-
-  if (GV->isDeclaration()) return false;
-
-  switch(GV->getLinkage()) {
-  default: llvm_unreachable("unknow linkage type");
-  case GlobalValue::AvailableExternallyLinkage:
-  case GlobalValue::ExternalWeakLinkage:
+  if (!GV->isStrongDefinitionForLinker())
     return false;
 
-  // Callee with weak linkage is allowed if it has hidden or protected
-  // visibility
-  case GlobalValue::LinkOnceAnyLinkage:
-  case GlobalValue::LinkOnceODRLinkage: // e.g. c++ inline functions
-  case GlobalValue::WeakAnyLinkage:
-  case GlobalValue::WeakODRLinkage:     // e.g. c++ template instantiation
-    if (GV->hasDefaultVisibility())
+  // Any explicitly-specified sections and section prefixes must also match.
+  // Also, if we're using -ffunction-sections, then each function is always in
+  // a different section (the same is true for COMDAT functions).
+  if (TM.getFunctionSections() || GV->hasComdat() || Caller->hasComdat() ||
+      GV->getSection() != Caller->getSection())
+    return false;
+  if (const auto *F = dyn_cast<Function>(GV)) {
+    if (F->getSectionPrefix() != Caller->getSectionPrefix())
       return false;
-
-  case GlobalValue::ExternalLinkage:
-  case GlobalValue::InternalLinkage:
-  case GlobalValue::PrivateLinkage:
-    break;
   }
 
-  // With '-fPIC', calling default visiblity function need insert 'nop' after
-  // function call, no matter that function resides in same module or not, so
-  // we treat it as in different module.
-  if (RelMod == Reloc::PIC_ && GV->hasDefaultVisibility())
+  // If the callee might be interposed, then we can't assume the ultimate call
+  // target will be in the same section.
+  if (GV->isInterposable() &&
+      !TM.shouldAssumeDSOLocal(*Caller->getParent(), GV))
     return false;
 
   return true;
 }
 
 static bool
 needStackSlotPassParameters(const PPCSubtarget &Subtarget,
                             const SmallVectorImpl<ISD::OutputArg> &Outs) {
PPCTargetLowering::IsEligibleForTailCallOptimization_64SVR4(
   if (any_of(Outs, [](const ISD::OutputArg& OA) { return OA.Flags.isByVal(); }))
     return false;
 
   // No TCO/SCO on indirect call because Caller have to restore its TOC
   if (!isFunctionGlobalAddress(Callee) &&
       !isa<ExternalSymbolSDNode>(Callee))
     return false;
 
   // Check if Callee resides in the same module, because for now, PPC64 SVR4 ABI
   // (ELFv1/ELFv2) doesn't allow tail calls to a symbol resides in another
   // module.
   // ref: https://bugzilla.mozilla.org/show_bug.cgi?id=973977
-  if (!resideInSameModule(Callee, getTargetMachine().getRelocationModel()))
+  if (!resideInSameSection(MF.getFunction(), Callee, getTargetMachine()))
     return false;
 
   // TCO allows altering callee ABI, so we don't have to check further.
   if (CalleeCC == CallingConv::Fast && TailCallOpt)
     return true;
 
   if (DisableSCO) return false;
 
PrepareCall(SelectionDAG &DAG, SDValue &Callee, SDValue &InFlag, SDValue &Chain,
   if (isSVR4ABI && isPPC64 && !isPatchPoint) {
     setUsesTOCBasePtr(DAG);
     Ops.push_back(DAG.getRegister(PPC::X2, PtrVT));
   }
 
   return CallOpc;
 }
 
-static
-bool isLocalCall(const SDValue &Callee)
-{
-  if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee))
-    return G->getGlobal()->isStrongDefinitionForLinker();
-  return false;
-}
-
 SDValue PPCTargetLowering::LowerCallResult(
     SDValue Chain, SDValue InFlag, CallingConv::ID CallConv, bool isVarArg,
     const SmallVectorImpl<ISD::InputArg> &Ins, const SDLoc &dl,
     SelectionDAG &DAG, SmallVectorImpl<SDValue> &InVals) const {
 
   SmallVector<CCValAssign, 16> RVLocs;
   CCState CCRetInfo(CallConv, isVarArg, DAG.getMachineFunction(), RVLocs,
                     *DAG.getContext());
SDValue PPCTargetLowering::FinishCall(
   // SVR4 ABI. At link time, if caller and callee are in a different module and
   // thus have a different TOC, the call will be replaced with a call to a stub
   // function which saves the current TOC, loads the TOC of the callee and
   // branches to the callee. The NOP will be replaced with a load instruction
   // which restores the TOC of the caller from the TOC save slot of the current
   // stack frame. If caller and callee belong to the same module (and have the
   // same TOC), the NOP will remain unchanged.
 
+  MachineFunction &MF = DAG.getMachineFunction();
   if (!isTailCall && Subtarget.isSVR4ABI()&& Subtarget.isPPC64() &&
       !isPatchPoint) {
     if (CallOpc == PPCISD::BCTRL) {
       // This is a call through a function pointer.
       // Restore the caller TOC from the save area into R2.
       // See PrepareCall() for more information about calls through function
       // pointers in the 64-bit SVR4 ABI.
       // We are using a target-specific load with r2 hard coded, because the
       // result of a target-independent load would never go directly into r2,
       // since r2 is a reserved register (which prevents the register allocator
       // from allocating it), resulting in an additional register being
       // allocated and an unnecessary move instruction being generated.
       CallOpc = PPCISD::BCTRL_LOAD_TOC;
 
       EVT PtrVT = getPointerTy(DAG.getDataLayout());
       SDValue StackPtr = DAG.getRegister(PPC::X1, PtrVT);
       unsigned TOCSaveOffset = Subtarget.getFrameLowering()->getTOCSaveOffset();
       SDValue TOCOff = DAG.getIntPtrConstant(TOCSaveOffset, dl);
       SDValue AddTOC = DAG.getNode(ISD::ADD, dl, MVT::i64, StackPtr, TOCOff);
 
       // The address needs to go after the chain input but before the flag (or
       // any other variadic arguments).
       Ops.insert(std::next(Ops.begin()), AddTOC);
-    } else if ((CallOpc == PPCISD::CALL) &&
-               (!isLocalCall(Callee) ||
-                DAG.getTarget().getRelocationModel() == Reloc::PIC_))
+    } else if (CallOpc == PPCISD::CALL &&
+      !resideInSameSection(MF.getFunction(), Callee, DAG.getTarget())) {
       // Otherwise insert NOP for non-local calls.
       CallOpc = PPCISD::CALL_NOP;
     }
+  }
 
   Chain = DAG.getNode(CallOpc, dl, NodeTys, Ops);
   InFlag = Chain.getValue(1);
 
   Chain = DAG.getCALLSEQ_END(Chain, DAG.getIntPtrConstant(NumBytes, dl, true),
                              DAG.getIntPtrConstant(BytesCalleePops, dl, true),
                              InFlag, dl);
   if (!Ins.empty())

test/CodeGen/PowerPC/ppc64-blnop.ll

+; RUN: llc < %s -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu | FileCheck %s
+; RUN: llc < %s -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr8 | FileCheck %s
+; RUN: llc < %s -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 | FileCheck %s
+; RUN: llc < %s -relocation-model=pic -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu | FileCheck %s
+; RUN: llc < %s -function-sections -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu | FileCheck %s -check-prefix=CHECK-FS
+
+%class.T = type { [2 x i8] }
+
+define void @e_callee(%class.T* %this, i8* %c) { ret void }
+define void @e_caller(%class.T* %this, i8* %c) {
+  call void @e_callee(%class.T* %this, i8* %c)
+  ret void
+
+; CHECK-LABEL: e_caller:
+; CHECK: bl e_callee
+; CHECK-NOT: nop
+
+; CHECK-FS-LABEL: e_caller:
+; CHECK-FS: bl e_callee
+; CHECK-FS-NEXT: nop
+}
+
+define void @e_scallee(%class.T* %this, i8* %c) section "different" { ret void }
+define void @e_scaller(%class.T* %this, i8* %c) {
+  call void @e_scallee(%class.T* %this, i8* %c)
+  ret void
+
+; CHECK-LABEL: e_scaller:
+; CHECK: bl e_scallee
+; CHECK-NEXT: nop
+}
+
+define void @e_s2callee(%class.T* %this, i8* %c) { ret void }
+define void @e_s2caller(%class.T* %this, i8* %c) section "different" {
+  call void @e_s2callee(%class.T* %this, i8* %c)
+  ret void
+
+; CHECK-LABEL: e_s2caller:
+; CHECK: bl e_s2callee
+; CHECK-NEXT: nop
+}
+
+$cd1 = comdat any
+$cd2 = comdat any
+
+define void @e_ccallee(%class.T* %this, i8* %c) comdat($cd1) { ret void }
+define void @e_ccaller(%class.T* %this, i8* %c) comdat($cd2) {
+  call void @e_ccallee(%class.T* %this, i8* %c)
+  ret void
+
+; CHECK-LABEL: e_ccaller:
+; CHECK: bl e_ccallee
+; CHECK-NEXT: nop
+}
+
+$cd = comdat any
+
+define void @e_c1callee(%class.T* %this, i8* %c) comdat($cd) { ret void }
+define void @e_c1caller(%class.T* %this, i8* %c) comdat($cd) {
+  call void @e_c1callee(%class.T* %this, i8* %c)
+  ret void
+
+; CHECK-LABEL: e_c1caller:
+; CHECK: bl e_c1callee
+; CHECK-NEXT: nop
+}
+
+define weak_odr hidden void @wo_hcallee(%class.T* %this, i8* %c) { ret void }
+define void @wo_hcaller(%class.T* %this, i8* %c) {
+  call void @wo_hcallee(%class.T* %this, i8* %c)
+  ret void
+
+; CHECK-LABEL: wo_hcaller:
+; CHECK: bl wo_hcallee
+; CHECK-NEXT: nop
+}
+
+define weak_odr protected void @wo_pcallee(%class.T* %this, i8* %c) { ret void }
+define void @wo_pcaller(%class.T* %this, i8* %c) {
+  call void @wo_pcallee(%class.T* %this, i8* %c)
+  ret void
+
+; CHECK-LABEL: wo_pcaller:
+; CHECK: bl wo_pcallee
+; CHECK-NEXT: nop
+}
+
+define weak_odr void @wo_callee(%class.T* %this, i8* %c) { ret void }
+define void @wo_caller(%class.T* %this, i8* %c) {
+  call void @wo_callee(%class.T* %this, i8* %c)
+  ret void
+
+; CHECK-LABEL: wo_caller:
+; CHECK: bl wo_callee
+; CHECK-NEXT: nop
+}
+
+define weak protected void @w_pcallee(i8* %ptr) { ret void }
+define void @w_pcaller(i8* %ptr) {
+  call void @w_pcallee(i8* %ptr)
+  ret void
+
+; CHECK-LABEL: w_pcaller:
+; CHECK: bl w_pcallee
+; CHECK-NEXT: nop
+}
+
+define weak hidden void @w_hcallee(i8* %ptr) { ret void }
+define void @w_hcaller(i8* %ptr) {
+  call void @w_hcallee(i8* %ptr)
+  ret void
+
+; CHECK-LABEL: w_hcaller:
+; CHECK: bl w_hcallee
+; CHECK-NEXT: nop
+}
+
+define weak void @w_callee(i8* %ptr) { ret void }
+define void @w_caller(i8* %ptr) {
+  call void @w_callee(i8* %ptr)
+  ret void
+
+; CHECK-LABEL: w_caller:
+; CHECK: bl w_callee
+; CHECK-NEXT: nop
+}
+

test/CodeGen/PowerPC/ppc64-sibcall.ll

 %class.T = type { [2 x i8] }
 
 define weak_odr hidden void @wo_hcallee(%class.T* %this, i8* %c) { ret void }
 define void @wo_hcaller(%class.T* %this, i8* %c) {
   tail call void @wo_hcallee(%class.T* %this, i8* %c)
   ret void
 
 ; CHECK-SCO-LABEL: wo_hcaller:
-; CHECK-SCO: b wo_hcallee
+; CHECK-SCO: bl wo_hcallee
 }
 
 define weak_odr protected void @wo_pcallee(%class.T* %this, i8* %c) { ret void }
 define void @wo_pcaller(%class.T* %this, i8* %c) {
   tail call void @wo_pcallee(%class.T* %this, i8* %c)
   ret void
 
 ; CHECK-SCO-LABEL: wo_pcaller:
-; CHECK-SCO: b wo_pcallee
+; CHECK-SCO: bl wo_pcallee
 }
 
 define weak_odr void @wo_callee(%class.T* %this, i8* %c) { ret void }
 define void @wo_caller(%class.T* %this, i8* %c) {
   tail call void @wo_callee(%class.T* %this, i8* %c)
   ret void
 
 ; CHECK-SCO-LABEL: wo_caller:
 ; CHECK-SCO: bl wo_callee
 }
 
 define weak protected void @w_pcallee(i8* %ptr) { ret void }
 define void @w_pcaller(i8* %ptr) {
   tail call void @w_pcallee(i8* %ptr)
   ret void
 
 ; CHECK-SCO-LABEL: w_pcaller:
-; CHECK-SCO: b w_pcallee
+; CHECK-SCO: bl w_pcallee
 }
 
 define weak hidden void @w_hcallee(i8* %ptr) { ret void }
 define void @w_hcaller(i8* %ptr) {
   tail call void @w_hcallee(i8* %ptr)
   ret void
 
 ; CHECK-SCO-LABEL: w_hcaller:
-; CHECK-SCO: b w_hcallee
+; CHECK-SCO: bl w_hcallee
 }
 
 define weak void @w_callee(i8* %ptr) { ret void }
 define void @w_caller(i8* %ptr) {
   tail call void @w_callee(i8* %ptr)
   ret void
 
 ; CHECK-SCO-LABEL: w_caller: