[PowerPC] Fix for PC Relative call protocol

[stefanp]

Summary

The situation where the caller uses a TOC and the callee does not but is marked as clobbers the TOC (st_other=1) was not being compiled correctly if both functions where in the same object file.
The call site where we had callee was missing a nop after the call. This is because it was assumed that since the two functions where in the same DSO they would be sharing a TOC. This is not the case if the callee uses PC Relative because in that case it may clobber the TOC.
This patch makes sure that we add the cnop correctly so that the linker has a place to restore the TOC.

[NeHuang]

This comment was removed by NeHuang.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4717–4721

nit: I think we should update the comment here as we are checking new scenarios below.

4739

The fix makes sense to me.

For the other two scenarios:

1. Caller using PCRelative addressing, callee using TOC.
2. Caller using PCRelative addressing, callee using PCRelative addressing.

In both cases, is it possible TOC could be clobbered in the caller (same as the test case below when it call external function externalFunc)? Can we say caller and callee are not sharing the same TOC base?
If so, caller and callee are not share the same TOC but we do not need nop to restore the TOC in the caller.

In this sense, shall we rename this function to callDoesNotRequireTOCRestore since the previous assumption no longer hold for PCRelative addressing?

[NeHuang]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4696–4697

Seems the assumption "calls share the same TOC base implies call does not require a toc restore" no longer hold when either caller or callee is using PCRelative addressing. Do you think it will be better to rename this function?

[NeHuang]

nit: In SUMMARY "add the cnop" to "add the nop"

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4739

please ignore this one and found the comment in line 4664

[stefanp]

Fixed the comment and changed the name of the function to have a better description of what it does.

[NeHuang]

LGTM

[sfertile]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4727

Could the GlobalValue be a GlobalindirectSymbol instead of a function?

4939

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.

[saghir]

LGTM

[sfertile]

llvm/test/CodeGen/PowerPC/pcrel-local-caller-toc.ll

9

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?

63

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.

[stefanp]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4727

Good point. I'll add another case for the GlobalIndirectSymbol as well.

4939

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.

5300

Anything PC Relative shouldn't get past this.

llvm/test/CodeGen/PowerPC/pcrel-local-caller-toc.ll

9

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.

63

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.

[stefanp]

Address Review Comments

[sfertile]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4701

If this is only used in the assert then it will causes a warning to be introduced in builds where asserts are disabled.

4727

Hmm, I think we want to try to look through GlobalAliases. Since they must alias a local definition we know the callee, can get it and determine if it uses PCRel or not. If the GlobalIndirectSymbol is an IFunc then the callee is determined at run-time by the dynamic linker calling the resolver function and I don't think we can determine any of the properties of the callee at compile time.

4939

I had missed that thank you. I think the assert you added is a good idea.

[sfertile]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4724

This check was split from the small code model one below since for medium and large code model it was a suffcient condition, while small code model it was only a necessary condition. Now its simply a necessary condition for all code models so we can colapse the 2 checks into a single check with return on false.

If you want some help combining the 2 long comments into one we can do so together in slack if you like.

4748

Do we only check this when targeting large/medium code model because PCRel calls is exclusive to medium code model? If the callee and caller already have different attributes, could they have differing code models as well? The case I'm thinking about would be if callee/caller are in different modules say in a lto or thin lto compile.

[stefanp]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4701

Good catch. I'll add an NDEBUG around it.

4748

It's true that PC Rel is only for medium code model. However, as you mentioned, the TOC caller code can be in any model. I'll move this PCRel checking code out of the if statement.

[stefanp]

Reworked the body of callsShareTOCBase to check for PC Relative callees in all cases.
Also, added test cases for ifunc and aliased functions. For the ifunc case I just added test lines to an existing test file.

[sfertile]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4772

I think we can pare down this comment.

The example does a good job at showing why its the callees pre-emptability that matters rather then if the caller/callee are in the same section. I'm not sure that in itself is useful enough to warrant a long comment in the code though. I think the relationship is straightforward enough:

If the callee is preemptable, then the static linker will use a plt-stub which saves the toc to the stack, and needs a nop after the call instruction to convert to a toc-restore.

In this context the given example needing a toc-restore or being un-tail-callable is obvious. I think we should replace the existing comment with something similar to what I used above.

4803

This check is redundant.

llvm/test/CodeGen/PowerPC/func-alias.ll

4 ↗

(On Diff #274038)

Minor nit: I think having P9 and P10 cases is enough coverage.

llvm/test/CodeGen/PowerPC/ifunc.ll

7 ↗

(On Diff #274038)

Minor nit: I don't think we need a P9 run. P8 and P10 are enough.

[stefanp]

Fixed up comment and test cases.
Removed condition that was not required.

[sfertile]

LGTM. Thank Stefan.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

4729

real minor nit: 'a valid Alias' --> 'an Alias'

[stefanp]

Fixed final nit.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

static int CalculateTailCallSPDiff(SelectionDAG& DAG, bool isTailCall,
   // Remember only if the new adjustment is bigger.
   if (SPDiff < FI->getTailCallSPDelta())
     FI->setTailCallSPDelta(SPDiff);
 
   return SPDiff;
 }
 
 static bool isFunctionGlobalAddress(SDValue Callee);
 
-static bool
-callsShareTOCBase(const Function *Caller, SDValue Callee,
+static bool callsShareTOCBase(const Function *Caller, SDValue Callee,
                               const TargetMachine &TM) {
-   // Callee is either a GlobalAddress or an ExternalSymbol. ExternalSymbols
-   // don't have enough information to determine if the caller and calle share
-   // the same  TOC base, so we have to pessimistically assume they don't for
-   // correctness.
-   GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee);
-   if (!G)
-     return false;
+  // It does not make sense to call callsShareTOCBase() with a caller that
+  // is PC Relative since PC Relative callers do not have a TOC.
+  const PPCSubtarget *STICaller = &TM.getSubtarget<PPCSubtarget>(*Caller);
+  assert(!STICaller->isUsingPCRelativeCalls() &&
+         "PC Relative callers do not have a TOC and cannot share a TOC Base");
+
 
-   const GlobalValue *GV = G->getGlobal();
   // The medium and large code models are expected to provide a sufficiently
   // large TOC to provide all data addressing needs of a module with a
   // single TOC. Since each module will be addressed with a single TOC then we
   // only need to check that caller and callee don't cross dso boundaries.
+  // In the case of PC Relative addressing this assumption is not always true.
+  // Functions with PC Relative enabled may clobber the TOC in the same DSO.
+  // We may need a TOC restore in the situation where the caller requires a
+  // valid TOC but the callee is PC Relative and does not.
   if (CodeModel::Medium == TM.getCodeModel() ||
-      CodeModel::Large == TM.getCodeModel())
-    return TM.shouldAssumeDSOLocal(*Caller->getParent(), GV);
+      CodeModel::Large == TM.getCodeModel()) {
+    if (!TM.shouldAssumeDSOLocal(*Caller->getParent(), GV))
+      return false;
+
+    const Function *F = dyn_cast<Function>(GV);
+    const GlobalIndirectSymbol *Sym = dyn_cast<GlobalIndirectSymbol>(GV);
+
+    // Either the function or the symbol needs to have a valid pointer.
+    // Ohterwise, there is no way to check the subtarget info for the callee.
+    if (!F && !Sym)
+      return false;
+
+    if (Sym) {
+      const GlobalObject *GlobalObj = Sym->getBaseObject();
+      F = dyn_cast<Function>(GlobalObj);
+    }
+
+    if (!F)
+      return false;
+
+    const PPCSubtarget *STICallee = &TM.getSubtarget<PPCSubtarget>(*F);
+
+    // If the callee uses PC Relative we cannot guarantee that the callee won't
+    // clobber the TOC of the caller and so we must assume that the two
+    // functions do not share a TOC base.
+    if (STICallee->isUsingPCRelativeCalls())
+      return false;
+
+    return true;
+  }
 
   // Otherwise we need to ensure callee and caller are in the same section,
   // since the linker may allocate multiple TOCs, and we don't know which
   // sections will belong to the same TOC base.
 
   if (!GV->isStrongDefinitionForLinker())
     return false;
 
   // 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;
   }
 
   // If the callee might be interposed, then we can't assume the ultimate call
   // target will be in the same section. Even in cases where we can assume that
   // interposition won't happen, in any case where the linker might insert a
   // stub to allow for interposition, we must generate code as though
   // interposition might occur. To understand why this matters, consider a
   // situation where: a -> b -> c where the arrows indicate calls. b and c are
   // in the same section, but a is in a different module (i.e. has a different
   // TOC base pointer). If the linker allows for interposition between b and c,
   // then it will generate a stub for the call edge between b and c which will

 static bool
 needStackSlotPassParameters(const PPCSubtarget &Subtarget,
                             const SmallVectorImpl<ISD::OutputArg> &Outs) {
   assert(Subtarget.is64BitELFABI());
 
   const unsigned PtrByteSize = 8;
   const unsigned LinkageSize = Subtarget.getFrameLowering()->getLinkageSize();
 
   static const MCPhysReg GPR[] = {
     PPC::X3, PPC::X4, PPC::X5, PPC::X6,
     PPC::X7, PPC::X8, PPC::X9, PPC::X10,
   };
   static const MCPhysReg VR[] = {
     PPC::V2, PPC::V3, PPC::V4, PPC::V5, PPC::V6, PPC::V7, PPC::V8,
     PPC::V9, PPC::V10, PPC::V11, PPC::V12, PPC::V13
   };
 
bool PPCTargetLowering::IsEligibleForTailCallOptimization_64SVR4(
   // When PC-Relative addressing is used, the concept of the TOC is no longer
   // applicable so this check is not required.
   // Check first for indirect calls.
   if (!Subtarget.isUsingPCRelativeCalls() &&
       !isFunctionGlobalAddress(Callee) && !isa<ExternalSymbolSDNode>(Callee))
     return false;
 
   // Check if we share the TOC base.
   if (!Subtarget.isUsingPCRelativeCalls() &&
       !callsShareTOCBase(&Caller, 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;
if (CFlags.IsIndirect) {
     // as it is not saved or used.
     return isTOCSaveRestoreRequired(Subtarget) ? PPCISD::BCTRL_LOAD_TOC
                                                : PPCISD::BCTRL;
   }
 
   if (Subtarget.isUsingPCRelativeCalls()) {
     assert(Subtarget.is64BitELFABI() && "PC Relative is only on ELF ABI.");
     return PPCISD::CALL_NOTOC;
   }
 
   // The ABIs that maintain a TOC pointer accross calls need to have a nop
   // immediately following the call instruction if the caller and callee may
   // have different TOC bases. At link time if the linker determines the calls
   // may not share a TOC base, the call is redirected to a trampoline inserted
   // by the linker. The trampoline will (among other things) save the callers
   // TOC pointer at an ABI designated offset in the linkage area and the linker
   // will rewrite the nop to be a load of the TOC pointer from the linkage area
   // into gpr2.
   if (Subtarget.isAIXABI() || Subtarget.is64BitELFABI())
-      return callsShareTOCBase(&Caller, Callee, TM) ? PPCISD::CALL
-                                                    : PPCISD::CALL_NOP;
 
   return PPCISD::CALL;
 }
 
 static SDValue transformCallee(const SDValue &Callee, SelectionDAG &DAG,
                                const SDLoc &dl, const PPCSubtarget &Subtarget) {
   if (!Subtarget.usesFunctionDescriptors() && !Subtarget.isELFv2ABI())
     if (SDNode *Dest = isBLACompatibleAddress(Callee, DAG))

llvm/test/CodeGen/PowerPC/pcrel-local-caller-toc.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:   -mcpu=future -ppc-asm-full-reg-names < %s | FileCheck %s
+
+; The purpose of this test is to check the call protocols for the situation
+; where the caller has PC Relative disabled, the callee has PC Relative
+; 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.
+
+@global = external local_unnamed_addr global i32, align 4
+
+define dso_local signext i32 @callee(i32 signext %a) local_unnamed_addr #0 {
+; CHECK-LABEL: callee:
+; CHECK:         .localentry callee, 1
+; CHECK-NEXT:  # %bb.0: # %entry
+; CHECK-NEXT:    mflr r0
+; CHECK-NEXT:    std r30, -16(r1) # 8-byte Folded Spill
+; CHECK-NEXT:    std r0, 16(r1)
+; CHECK-NEXT:    stdu r1, -48(r1)
+; CHECK-NEXT:    mr r30, r3
+; CHECK-NEXT:    bl externalFunc@notoc
+; CHECK-NEXT:    add r3, r3, r30
+; CHECK-NEXT:    extsw r3, r3
+; CHECK-NEXT:    addi r1, r1, 48
+; CHECK-NEXT:    ld r0, 16(r1)
+; CHECK-NEXT:    ld r30, -16(r1) # 8-byte Folded Reload
+; CHECK-NEXT:    mtlr r0
+; CHECK-NEXT:    blr
+entry:
+  %call = tail call signext i32 @externalFunc(i32 signext %a) #3
+  %add = add nsw i32 %call, %a
+  ret i32 %add
+}
+
+declare signext i32 @externalFunc(i32 signext) local_unnamed_addr #1
+
+define dso_local void @caller(i32 signext %a) local_unnamed_addr #2 {
+; CHECK-LABEL: caller:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    mflr r0
+; CHECK-NEXT:    std r30, -16(r1) # 8-byte Folded Spill
+; CHECK-NEXT:    std r0, 16(r1)
+; CHECK-NEXT:    stdu r1, -48(r1)
+; CHECK-NEXT:    addis r4, r2, .LC0@toc@ha
+; CHECK-NEXT:    ld r30, .LC0@toc@l(r4)
+; CHECK-NEXT:    lwz r4, 0(r30)
+; CHECK-NEXT:    add r3, r4, r3
+; CHECK-NEXT:    extsw r3, r3
+; CHECK-NEXT:    bl callee
+; CHECK-NEXT:    nop
+; CHECK-NEXT:    mullw r3, r3, r3
+; CHECK-NEXT:    stw r3, 0(r30)
+; CHECK-NEXT:    addi r1, r1, 48
+; CHECK-NEXT:    ld r0, 16(r1)
+; CHECK-NEXT:    ld r30, -16(r1) # 8-byte Folded Reload
+; CHECK-NEXT:    mtlr r0
+; CHECK-NEXT:    blr
+entry:
+  %0 = load i32, i32* @global, align 4
+  %add = add nsw i32 %0, %a
+  %call = tail call signext i32 @callee(i32 signext %add)
+  %mul = mul nsw i32 %call, %call
+  store i32 %mul, i32* @global, align 4
+  ret void
+}
+
+define dso_local signext i32 @tail_caller(i32 signext %a) local_unnamed_addr #2 {
+; CHECK-LABEL: tail_caller:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    mflr r0
+; CHECK-NEXT:    std r0, 16(r1)
+; CHECK-NEXT:    stdu r1, -32(r1)
+; CHECK-NEXT:    addis r4, r2, .LC0@toc@ha
+; CHECK-NEXT:    ld r4, .LC0@toc@l(r4)
+; CHECK-NEXT:    lwz r4, 0(r4)
+; CHECK-NEXT:    add r3, r4, r3
+; CHECK-NEXT:    extsw r3, r3
+; CHECK-NEXT:    bl callee
+; CHECK-NEXT:    nop
+; CHECK-NEXT:    addi r1, r1, 32
+; CHECK-NEXT:    ld r0, 16(r1)
+; CHECK-NEXT:    mtlr r0
+; CHECK-NEXT:    blr
+entry:
+  %0 = load i32, i32* @global, align 4
+  %add = add nsw i32 %0, %a
+  %call = tail call signext i32 @callee(i32 signext %add)
+  ret i32 %call
+}
+
+
+; Left the target features in this test because it is important that caller has
+; -pcrelative-memops while callee has +pcrelative-memops
+attributes #0 = { nounwind "target-features"="+altivec,+bpermd,+crypto,+direct-move,+extdiv,+pcrelative-memops,+power8-vector,+power9-vector,+vsx,-htm,-qpx,-spe" }
+attributes #1 = { "target-features"="+altivec,+bpermd,+crypto,+direct-move,+extdiv,+pcrelative-memops,+power8-vector,+power9-vector,+vsx,-htm,-qpx,-spe" }
+attributes #2 = { nounwind "target-features"="+altivec,+bpermd,+crypto,+direct-move,+extdiv,+power8-vector,+power9-vector,+vsx,-htm,-pcrelative-memops,-qpx,-spe" }
+attributes #3 = { nounwind }