[llvm] r315680 - Revert r314923: "Recommit : Use the basic cost if a GEP is not used as addressing mode"
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 13 10:55:13 PDT 2017
On Fri, Oct 13, 2017 at 10:21 AM Jun Lim via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
>
> r314923 could potentially increase the inline cost because with this
> change getGEPCost will no longer returns Free for a GEP which is actually a
> non-free. I doubt simply reverting is the right decision because this
> change fixed the unexpected behavior in getGEPCost() and caused the
> regression in the benchmark of which performance reply on the incorrect
> cost value. If we are doing right thing with r314923, we should look at
> other way arounds to get the hot function inlined.
>
Reverting to green is the right call here. The regression on Gipfeli wasn't
a 1% or 2% regression, it was a 15% to 20% regression. We can't even
continue testing the compiler with that magnitude of regression.
I'll follow-up on the actual review thread with thoughts about why this is
problematic. But I think going back to a known working prior state is
really the right call.
>
>
>
>
>
>
> *From:* Daniel Jasper [mailto:djasper at google.com]
> *Sent:* Friday, October 13, 2017 1:00 PM
> *To:* llvm-commits <llvm-commits at lists.llvm.org>; Jun Bum Lim <
> junbuml at codeaurora.org>; Benjamin Kramer <benny.kra at googlemail.com>
> *Subject:* Re: [llvm] r315680 - Revert r314923: "Recommit : Use the basic
> cost if a GEP is not used as addressing mode"
>
>
>
> We are still investigating, but one of the main differences seems to be
> that LLVM stops inlining AppendFromSelf:
>
> https://github.com/google/gipfeli/blob/master/stream.h#L90
>
>
>
>
>
>
>
> On Fri, Oct 13, 2017 at 3:04 PM, Daniel Jasper via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Author: djasper
> Date: Fri Oct 13 07:04:21 2017
> New Revision: 315680
>
> URL: http://llvm.org/viewvc/llvm-project?rev=315680&view=rev
> Log:
> Revert r314923: "Recommit : Use the basic cost if a GEP is not used as
> addressing mode"
>
> Significantly reduces performancei (~30%) of gipfeli
> (https://github.com/google/gipfeli)
>
> I have not yet managed to reproduce this regression with the open-source
> version of the benchmark on github, but will work with others to get a
> reproducer to you later today.
>
> Removed:
> llvm/trunk/test/Transforms/SimplifyCFG/SpeculativeExecGepCE.ll
> Modified:
> llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h
> llvm/trunk/include/llvm/Analysis/TargetTransformInfoImpl.h
> llvm/trunk/include/llvm/CodeGen/BasicTTIImpl.h
> llvm/trunk/include/llvm/IR/Operator.h
> llvm/trunk/lib/Analysis/TargetTransformInfo.cpp
> llvm/trunk/lib/Transforms/Scalar/NaryReassociate.cpp
> llvm/trunk/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp
> llvm/trunk/test/Analysis/CostModel/AArch64/gep.ll
> llvm/trunk/test/Analysis/CostModel/X86/vector_gep.ll
>
> Modified: llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h?rev=315680&r1=315679&r2=315680&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h (original)
> +++ llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h Fri Oct 13
> 07:04:21 2017
> @@ -193,13 +193,6 @@ public:
> int getGEPCost(Type *PointeeType, const Value *Ptr,
> ArrayRef<const Value *> Operands) const;
>
> - /// \brief Estimate the cost of a GEP operation when lowered.
> - ///
> - /// This user-based overload adds the ability to check if the GEP can be
> - /// folded into its users.
> - int getGEPCost(const GEPOperator *GEP,
> - ArrayRef<const Value *> Operands) const;
> -
> /// \brief Estimate the cost of a EXT operation when lowered.
> ///
> /// The contract for this function is the same as \c getOperationCost
> except
> @@ -258,9 +251,9 @@ public:
> /// \brief Estimate the cost of a given IR user when lowered.
> ///
> /// This can estimate the cost of either a ConstantExpr or Instruction
> when
> - /// lowered. It has two primary advantages over the \c getOperationCost
> above,
> - /// and one significant disadvantage: it can only be used when the IR
> - /// construct has already been formed.
> + /// lowered. It has two primary advantages over the \c getOperationCost
> and
> + /// \c getGEPCost above, and one significant disadvantage: it can only
> be
> + /// used when the IR construct has already been formed.
> ///
> /// The advantages are that it can inspect the SSA use graph to reason
> more
> /// accurately about the cost. For example, all-constant-GEPs can often
> be
> @@ -939,8 +932,6 @@ public:
> virtual int getOperationCost(unsigned Opcode, Type *Ty, Type *OpTy) = 0;
> virtual int getGEPCost(Type *PointeeType, const Value *Ptr,
> ArrayRef<const Value *> Operands) = 0;
> - virtual int getGEPCost(const GEPOperator *GEP,
> - ArrayRef<const Value *> Operands) = 0;
> virtual int getExtCost(const Instruction *I, const Value *Src) = 0;
> virtual int getCallCost(FunctionType *FTy, int NumArgs) = 0;
> virtual int getCallCost(const Function *F, int NumArgs) = 0;
> @@ -1122,10 +1113,6 @@ public:
> ArrayRef<const Value *> Operands) override {
> return Impl.getGEPCost(PointeeType, Ptr, Operands);
> }
> - int getGEPCost(const GEPOperator *GEP,
> - ArrayRef<const Value *> Operands) override {
> - return Impl.getGEPCost(GEP, Operands);
> - }
> int getExtCost(const Instruction *I, const Value *Src) override {
> return Impl.getExtCost(I, Src);
> }
>
> Modified: llvm/trunk/include/llvm/Analysis/TargetTransformInfoImpl.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/TargetTransformInfoImpl.h?rev=315680&r1=315679&r2=315680&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/TargetTransformInfoImpl.h (original)
> +++ llvm/trunk/include/llvm/Analysis/TargetTransformInfoImpl.h Fri Oct 13
> 07:04:21 2017
> @@ -728,38 +728,6 @@ public:
> return TTI::TCC_Basic;
> }
>
> - int getGEPCost(const GEPOperator *GEP, ArrayRef<const Value *>
> Operands) {
> - if (!isa<Instruction>(GEP))
> - return TTI::TCC_Basic;
> -
> - Type *PointeeType = GEP->getSourceElementType();
> - const Value *Ptr = GEP->getPointerOperand();
> -
> - if (getGEPCost(PointeeType, Ptr, Operands) == TTI::TCC_Free) {
> - // Should check if the GEP is actually used in load / store
> instructions.
> - // For simplicity, we check only direct users of the GEP.
> - //
> - // FIXME: GEPs could also be folded away as a part of addressing
> mode in
> - // load/store instructions together with other instructions (e.g.,
> other
> - // GEPs). Handling all such cases must be expensive to be performed
> - // in this function, so we stay conservative for now.
> - for (const User *U : GEP->users()) {
> - const Operator *UOP = cast<Operator>(U);
> - const Value *PointerOperand = nullptr;
> - if (auto *LI = dyn_cast<LoadInst>(UOP))
> - PointerOperand = LI->getPointerOperand();
> - else if (auto *SI = dyn_cast<StoreInst>(UOP))
> - PointerOperand = SI->getPointerOperand();
> -
> - if ((!PointerOperand || PointerOperand != GEP) &&
> - !GEP->hasAllZeroIndices())
> - return TTI::TCC_Basic;
> - }
> - return TTI::TCC_Free;
> - }
> - return TTI::TCC_Basic;
> - }
> -
> using BaseT::getIntrinsicCost;
>
> unsigned getIntrinsicCost(Intrinsic::ID IID, Type *RetTy,
> @@ -783,9 +751,11 @@ public:
> if (A->isStaticAlloca())
> return TTI::TCC_Free;
>
> - if (const GEPOperator *GEP = dyn_cast<GEPOperator>(U))
> - return static_cast<T *>(this)->getGEPCost(GEP,
> + if (const GEPOperator *GEP = dyn_cast<GEPOperator>(U)) {
> + return static_cast<T
> *>(this)->getGEPCost(GEP->getSourceElementType(),
> + GEP->getPointerOperand(),
> Operands.drop_front());
> + }
>
> if (auto CS = ImmutableCallSite(U)) {
> const Function *F = CS.getCalledFunction();
>
> Modified: llvm/trunk/include/llvm/CodeGen/BasicTTIImpl.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/BasicTTIImpl.h?rev=315680&r1=315679&r2=315680&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/BasicTTIImpl.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/BasicTTIImpl.h Fri Oct 13 07:04:21 2017
> @@ -189,11 +189,6 @@ public:
> return BaseT::getGEPCost(PointeeType, Ptr, Operands);
> }
>
> - int getGEPCost(const GEPOperator *GEP,
> - ArrayRef<const Value *> Operands) {
> - return BaseT::getGEPCost(GEP, Operands);
> - }
> -
> int getExtCost(const Instruction *I, const Value *Src) {
> if (getTLI()->isExtFree(I))
> return TargetTransformInfo::TCC_Free;
>
> Modified: llvm/trunk/include/llvm/IR/Operator.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Operator.h?rev=315680&r1=315679&r2=315680&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/Operator.h (original)
> +++ llvm/trunk/include/llvm/IR/Operator.h Fri Oct 13 07:04:21 2017
> @@ -456,8 +456,6 @@ public:
> if (ConstantInt *C = dyn_cast<ConstantInt>(I))
> if (C->isZero())
> continue;
> - if (isa<ConstantAggregateZero>(I))
> - continue;
> return false;
> }
> return true;
>
> Modified: llvm/trunk/lib/Analysis/TargetTransformInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/TargetTransformInfo.cpp?rev=315680&r1=315679&r2=315680&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Analysis/TargetTransformInfo.cpp (original)
> +++ llvm/trunk/lib/Analysis/TargetTransformInfo.cpp Fri Oct 13 07:04:21
> 2017
> @@ -88,11 +88,6 @@ int TargetTransformInfo::getGEPCost(Type
> return TTIImpl->getGEPCost(PointeeType, Ptr, Operands);
> }
>
> -int TargetTransformInfo::getGEPCost(const GEPOperator *GEP,
> - ArrayRef<const Value *> Operands)
> const {
> - return TTIImpl->getGEPCost(GEP, Operands);
> -}
> -
> int TargetTransformInfo::getExtCost(const Instruction *I,
> const Value *Src) const {
> return TTIImpl->getExtCost(I, Src);
>
> Modified: llvm/trunk/lib/Transforms/Scalar/NaryReassociate.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NaryReassociate.cpp?rev=315680&r1=315679&r2=315680&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/NaryReassociate.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/NaryReassociate.cpp Fri Oct 13
> 07:04:21 2017
> @@ -264,7 +264,7 @@ static bool isGEPFoldable(GetElementPtrI
> SmallVector<const Value*, 4> Indices;
> for (auto I = GEP->idx_begin(); I != GEP->idx_end(); ++I)
> Indices.push_back(*I);
> - return TTI->getGEPCost(cast<GEPOperator>(GEP),
> + return TTI->getGEPCost(GEP->getSourceElementType(),
> GEP->getPointerOperand(),
> Indices) == TargetTransformInfo::TCC_Free;
> }
>
>
> Modified: llvm/trunk/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp?rev=315680&r1=315679&r2=315680&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp
> (original)
> +++ llvm/trunk/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp Fri
> Oct 13 07:04:21 2017
> @@ -239,7 +239,7 @@ static bool isGEPFoldable(GetElementPtrI
> SmallVector<const Value*, 4> Indices;
> for (auto I = GEP->idx_begin(); I != GEP->idx_end(); ++I)
> Indices.push_back(*I);
> - return TTI->getGEPCost(cast<GEPOperator>(GEP),
> + return TTI->getGEPCost(GEP->getSourceElementType(),
> GEP->getPointerOperand(),
> Indices) == TargetTransformInfo::TCC_Free;
> }
>
>
> Modified: llvm/trunk/test/Analysis/CostModel/AArch64/gep.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/CostModel/AArch64/gep.ll?rev=315680&r1=315679&r2=315680&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Analysis/CostModel/AArch64/gep.ll (original)
> +++ llvm/trunk/test/Analysis/CostModel/AArch64/gep.ll Fri Oct 13 07:04:21
> 2017
> @@ -290,49 +290,3 @@ define i64 @test36(i64* %p) {
> %v = load i64, i64* %a
> ret i64 %v
> }
> -
> -; CHECK-LABEL: test37
> -; CHECK: cost of 1 for instruction: {{.*}} = getelementptr inbounds i8*,
> i8**
> -define i8 @test37(i64 %j, i8** readonly %P) {
> -entry:
> - %arrayidx0 = getelementptr inbounds i8*, i8** %P, i64 %j
> - %l1 = call i8* @func(i8** %arrayidx0)
> - ret i8 0
> -}
> -
> -; CHECK-LABEL: test38
> -; CHECK: cost of 1 for instruction: {{.*}} = getelementptr inbounds i8*,
> i8**
> -define i8 @test38(i8** readonly %P) {
> -entry:
> - %arrayidx0 = getelementptr inbounds i8*, i8** %P, i64 10
> - %l1 = call i8* @func(i8** %arrayidx0)
> - ret i8 0
> -}
> -
> -; CHECK-LABEL:test39
> -; CHECK: cost of 0 for instruction: {{.*}} = getelementptr inbounds i8*,
> i8**
> -define i8 @test39(i8** readonly %P) {
> -entry:
> - %arrayidx0 = getelementptr inbounds i8*, i8** %P, i64 0
> - %l1 = call i8* @func(i8** %arrayidx0)
> - ret i8 0
> -}
> -
> -; CHECK-LABEL:test40
> -; CHECK: cost of 1 for instruction: {{.*}} = getelementptr inbounds i8*,
> i8**
> -define i8** @test40(i8** readonly %P) {
> -entry:
> - %arrayidx0 = getelementptr inbounds i8*, i8** %P, i64 10
> - ret i8** %arrayidx0
> -}
> -
> -; CHECK-LABEL:test41
> -; CHECK: cost of 1 for instruction: {{.*}} = getelementptr inbounds i8,
> i8*
> -define i8 @test41(i8* %V, i8** readonly %P) {
> -entry:
> - %arrayidx0 = getelementptr inbounds i8, i8* %V, i64 10
> - store i8* %arrayidx0, i8** %P
> - ret i8 0
> -}
> -
> -declare i8* @func(i8**)
>
> Modified: llvm/trunk/test/Analysis/CostModel/X86/vector_gep.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/CostModel/X86/vector_gep.ll?rev=315680&r1=315679&r2=315680&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Analysis/CostModel/X86/vector_gep.ll (original)
> +++ llvm/trunk/test/Analysis/CostModel/X86/vector_gep.ll Fri Oct 13
> 07:04:21 2017
> @@ -10,7 +10,7 @@ define <4 x i32> @foov(<4 x %struct.S*>
> %vector = shufflevector <4 x i64> %temp, <4 x i64> undef, <4 x i32>
> zeroinitializer
> ;CHECK: cost of 0 for instruction: {{.*}} getelementptr inbounds %struct.S
> %B = getelementptr inbounds %struct.S, <4 x %struct.S*> %s, <4 x i32>
> zeroinitializer, <4 x i32> zeroinitializer
> -;CHECK: cost of 1 for instruction: {{.*}} getelementptr inbounds [1000 x
> i32]
> +;CHECK: cost of 0 for instruction: {{.*}} getelementptr inbounds [1000 x
> i32]
> %arrayidx = getelementptr inbounds [1000 x i32], <4 x [1000 x i32]*>
> %B, <4 x i64> zeroinitializer, <4 x i64> %vector
> %res = call <4 x i32> @llvm.masked.gather.v4i32.v4p0i32(<4 x i32*>
> %arrayidx, i32 4, <4 x i1> <i1 true, i1 true, i1 true, i1 true>, <4 x i32>
> undef)
> ret <4 x i32> %res
>
> Removed: llvm/trunk/test/Transforms/SimplifyCFG/SpeculativeExecGepCE.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/SpeculativeExecGepCE.ll?rev=315679&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/SimplifyCFG/SpeculativeExecGepCE.ll
> (original)
> +++ llvm/trunk/test/Transforms/SimplifyCFG/SpeculativeExecGepCE.ll
> (removed)
> @@ -1,28 +0,0 @@
> -; RUN: opt < %s -simplifycfg -phi-node-folding-threshold=0 -S | FileCheck
> %s
> -
> -target triple = "x86_64-unknown-linux-gnu"
> -
> - at d_buf = internal constant [8 x i8] [i8 126, i8 127, i8 128, i8 129, i8
> 130, i8 131, i8 132, i8 133], align 8
> - at a = internal constant { i8*, i64} {i8* getelementptr inbounds ([8 x i8],
> [8 x i8]* @d_buf, i64 0, i64 0), i64 0}
> -
> -; CHECK-LABEL: @test
> -; CHECK-LABEL: end:
> -; CHECK: %x1 = phi i8*
> -define i8* @test(i1* %dummy, i8* %a, i8* %b, i8 %v) {
> -
> -entry:
> - %cond1 = load volatile i1, i1* %dummy
> - br i1 %cond1, label %if, label %end
> -
> -if:
> - %cond2 = load volatile i1, i1* %dummy
> - br i1 %cond2, label %then, label %end
> -
> -then:
> - br label %end
> -
> -end:
> - %x1 = phi i8* [ %a, %entry ], [ %b, %if ], [getelementptr inbounds ([8
> x i8], [8 x i8]* @d_buf, i64 0, i64 0) , %then ]
> -
> - ret i8* %x1
> -}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171013/5a665592/attachment.html>
More information about the llvm-commits
mailing list