From 05e80d31bddd62fc34a47f9aac275fb0ca896c0a Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Fri, 16 Dec 2016 07:31:20 +0000 Subject: [PATCH] Revert r289638: [PowerPC] Fix logic dealing with nop after calls (and tail-call eligibility) This patch appears to result in trampolines in vtables being miscompiled when they in turn tail call a method. I've posted some preliminary details about the failure on the thread for this commit and talked to Hal. He was comfortable going ahead and reverting until we sort out what is wrong. llvm-svn: 289928 --- llvm/lib/Target/PowerPC/PPCISelLowering.cpp | 65 ++++++++------ llvm/test/CodeGen/PowerPC/ppc64-blnop.ll | 129 ---------------------------- llvm/test/CodeGen/PowerPC/ppc64-sibcall.ll | 8 +- 3 files changed, 44 insertions(+), 158 deletions(-) delete mode 100644 llvm/test/CodeGen/PowerPC/ppc64-blnop.ll diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp index 15765bc..aa3ffde 100644 --- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp +++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp @@ -3981,32 +3981,40 @@ static int CalculateTailCallSPDiff(SelectionDAG& DAG, bool isTailCall, static bool isFunctionGlobalAddress(SDValue Callee); static bool -resideInSameSection(const Function *Caller, SDValue Callee, - const TargetMachine &TM) { +resideInSameModule(SDValue Callee, Reloc::Model RelMod) { // If !G, Callee can be an external symbol. GlobalAddressSDNode *G = dyn_cast(Callee); - if (!G) - return false; + if (!G) return false; const GlobalValue *GV = G->getGlobal(); - 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()) + if (GV->isDeclaration()) return false; + + switch(GV->getLinkage()) { + default: llvm_unreachable("unknow linkage type"); + case GlobalValue::AvailableExternallyLinkage: + case GlobalValue::ExternalWeakLinkage: return false; - if (const auto *F = dyn_cast(GV)) { - if (F->getSectionPrefix() != Caller->getSectionPrefix()) + + // 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()) return false; + + case GlobalValue::ExternalLinkage: + case GlobalValue::InternalLinkage: + case GlobalValue::PrivateLinkage: + break; } - // 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)) + // 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()) return false; return true; @@ -4122,11 +4130,11 @@ PPCTargetLowering::IsEligibleForTailCallOptimization_64SVR4( !isa(Callee)) return false; - // Check if Callee resides in the same section, because for now, PPC64 SVR4 - // ABI (ELFv1/ELFv2) doesn't allow tail calls to a symbol resides in another - // section. + // 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 (!resideInSameSection(MF.getFunction(), Callee, getTargetMachine())) + if (!resideInSameModule(Callee, getTargetMachine().getRelocationModel())) return false; // TCO allows altering callee ABI, so we don't have to check further. @@ -4584,6 +4592,14 @@ PrepareCall(SelectionDAG &DAG, SDValue &Callee, SDValue &InFlag, SDValue &Chain, return CallOpc; } +static +bool isLocalCall(const SDValue &Callee) +{ + if (GlobalAddressSDNode *G = dyn_cast(Callee)) + return G->getGlobal()->isStrongDefinitionForLinker(); + return false; +} + SDValue PPCTargetLowering::LowerCallResult( SDValue Chain, SDValue InFlag, CallingConv::ID CallConv, bool isVarArg, const SmallVectorImpl &Ins, const SDLoc &dl, @@ -4685,7 +4701,6 @@ SDValue PPCTargetLowering::FinishCall( // 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) { @@ -4709,11 +4724,11 @@ SDValue PPCTargetLowering::FinishCall( // 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 && - !resideInSameSection(MF.getFunction(), Callee, DAG.getTarget())) { + } else if ((CallOpc == PPCISD::CALL) && + (!isLocalCall(Callee) || + DAG.getTarget().getRelocationModel() == Reloc::PIC_)) // Otherwise insert NOP for non-local calls. CallOpc = PPCISD::CALL_NOP; - } } Chain = DAG.getNode(CallOpc, dl, NodeTys, Ops); diff --git a/llvm/test/CodeGen/PowerPC/ppc64-blnop.ll b/llvm/test/CodeGen/PowerPC/ppc64-blnop.ll deleted file mode 100644 index 384edf3..0000000 --- a/llvm/test/CodeGen/PowerPC/ppc64-blnop.ll +++ /dev/null @@ -1,129 +0,0 @@ -; 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 -; RUN: llc < %s -relocation-model=pic -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu | FileCheck %s -; RUN: llc < %s -function-sections -verify-machineinstrs -mtriple=powerpc64le-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 -} - diff --git a/llvm/test/CodeGen/PowerPC/ppc64-sibcall.ll b/llvm/test/CodeGen/PowerPC/ppc64-sibcall.ll index 59e5456..418b782 100644 --- a/llvm/test/CodeGen/PowerPC/ppc64-sibcall.ll +++ b/llvm/test/CodeGen/PowerPC/ppc64-sibcall.ll @@ -142,7 +142,7 @@ define void @wo_hcaller(%class.T* %this, i8* %c) { ret void ; CHECK-SCO-LABEL: wo_hcaller: -; CHECK-SCO: bl wo_hcallee +; CHECK-SCO: b wo_hcallee } define weak_odr protected void @wo_pcallee(%class.T* %this, i8* %c) { ret void } @@ -151,7 +151,7 @@ define void @wo_pcaller(%class.T* %this, i8* %c) { ret void ; CHECK-SCO-LABEL: wo_pcaller: -; CHECK-SCO: bl wo_pcallee +; CHECK-SCO: b wo_pcallee } define weak_odr void @wo_callee(%class.T* %this, i8* %c) { ret void } @@ -169,7 +169,7 @@ define void @w_pcaller(i8* %ptr) { ret void ; CHECK-SCO-LABEL: w_pcaller: -; CHECK-SCO: bl w_pcallee +; CHECK-SCO: b w_pcallee } define weak hidden void @w_hcallee(i8* %ptr) { ret void } @@ -178,7 +178,7 @@ define void @w_hcaller(i8* %ptr) { ret void ; CHECK-SCO-LABEL: w_hcaller: -; CHECK-SCO: bl w_hcallee +; CHECK-SCO: b w_hcallee } define weak void @w_callee(i8* %ptr) { ret void } -- 2.7.4