[llvm] r278161 - Fix the runtime error caused by "Use ValueOffsetPair to enhance value reuse during SCEV expansion".
Wei Mi via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 29 11:19:04 PDT 2016
Steven, thanks for providing the testcase. I can reproduce the error.
Will look at it.
Thanks,
Wei.
On Mon, Aug 29, 2016 at 11:11 AM, Steven Wu <stevenwu at apple.com> wrote:
> Hi Wei
>
> I found a fallout from this commit. Here is a reduced testcase:
>
> $ cat bugpoint-reduced-simplified.ll
> target datalayout = "e-m:o-p:32:32-f64:32:64-f80:128-n8:16:32-S128"
> target triple = "i386-apple-macosx10.12.0"
>
> ; Function Attrs: nounwind optsize ssp
> define void @Foo() {
> entry:
> switch i2 undef, label %sw.epilog102 [
> i2 -2, label %sw.bb28
> ]
>
> sw.bb28: ; preds = %entry
> %0 = load i8*, i8** undef, align 2
> %add.ptr1.i = getelementptr inbounds i8, i8* undef, i32 -1
> %add.ptr4.i = getelementptr inbounds i8, i8* %0, i32 -1
> %cmp.i = icmp ult i8* undef, %0
> %add.ptr4.add.ptr1.i = select i1 %cmp.i, i8* %add.ptr4.i, i8* %add.ptr1.i
> br label %while.cond.i
>
> while.cond.i: ; preds = %while.cond.i, %sw.bb28
> %currPtr.1.i = phi i8* [ %incdec.ptr.i, %while.cond.i ], [ %add.ptr4.add.ptr1.i, %sw.bb28 ]
> %incdec.ptr.i = getelementptr inbounds i8, i8* %currPtr.1.i, i32 1
> %1 = load i8, i8* %incdec.ptr.i, align 1
> br label %while.cond.i
>
> sw.epilog102: ; preds = %entry
> unreachable
> }
>
> $ bugpoint-reduced-simplified.ll -loop-reduce -disable-output
> Assertion failed: (S1->getType() == S2->getType() && "Cannot create binary operator with two operands of differing type!"), function Create, file /Users/steven/dev/trunk/llvm/lib/IR/Instructions.cpp, line 2137.
>
> It seems SCEV trying to fold i8* %add.ptr4.i into *i8 subtract i32 1 which causes the assertion. Can you take a look?
>
> Thanks
>
> Steven
>
>> On Aug 9, 2016, at 1:40 PM, Wei Mi via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: wmi
>> Date: Tue Aug 9 15:40:03 2016
>> New Revision: 278161
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=278161&view=rev
>> Log:
>> Fix the runtime error caused by "Use ValueOffsetPair to enhance value reuse during SCEV expansion".
>>
>> The patch is to fix the bug in PR28705. It was caused by setting wrong return
>> value for SCEVExpander::findExistingExpansion. The return values of findExistingExpansion
>> have different meanings when the function is used in different ways so it is easy to make
>> mistake. The fix creates two new interfaces to replace SCEVExpander::findExistingExpansion,
>> and specifies where each interface is expected to be used.
>>
>> Differential Revision: https://reviews.llvm.org/D22942
>>
>> Added:
>> llvm/trunk/test/Analysis/ScalarEvolution/pr28705.ll
>> Modified:
>> llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h
>> llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp
>> llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp
>>
>> Modified: llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h?rev=278161&r1=278160&r2=278161&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h (original)
>> +++ llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h Tue Aug 9 15:40:03 2016
>> @@ -14,6 +14,7 @@
>> #ifndef LLVM_ANALYSIS_SCALAREVOLUTIONEXPANDER_H
>> #define LLVM_ANALYSIS_SCALAREVOLUTIONEXPANDER_H
>>
>> +#include "llvm/ADT/Optional.h"
>> #include "llvm/Analysis/ScalarEvolutionExpressions.h"
>> #include "llvm/Analysis/ScalarEvolutionNormalization.h"
>> #include "llvm/Analysis/TargetFolder.h"
>> @@ -268,7 +269,15 @@ namespace llvm {
>>
>> void setChainedPhi(PHINode *PN) { ChainedPhis.insert(PN); }
>>
>> - /// \brief Try to find LLVM IR value for S available at the point At.
>> + /// Try to find existing LLVM IR value for S available at the point At.
>> + Value *getExactExistingExpansion(const SCEV *S, const Instruction *At,
>> + Loop *L);
>> +
>> + /// Try to find the ValueOffsetPair for S. The function is mainly
>> + /// used to check whether S can be expanded cheaply.
>> + /// If this returns a non-None value, we know we can codegen the
>> + /// `ValueOffsetPair` into a suitable expansion identical with S
>> + /// so that S can be expanded cheaply.
>> ///
>> /// L is a hint which tells in which loop to look for the suitable value.
>> /// On success return value which is equivalent to the expanded S at point
>> @@ -276,7 +285,9 @@ namespace llvm {
>> ///
>> /// Note that this function does not perform an exhaustive search. I.e if it
>> /// didn't find any value it does not mean that there is no such value.
>> - Value *findExistingExpansion(const SCEV *S, const Instruction *At, Loop *L);
>> + ///
>> + Optional<ScalarEvolution::ValueOffsetPair>
>> + getRelatedExistingExpansion(const SCEV *S, const Instruction *At, Loop *L);
>>
>> private:
>> LLVMContext &getContext() const { return SE.getContext(); }
>>
>> Modified: llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp?rev=278161&r1=278160&r2=278161&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp (original)
>> +++ llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp Tue Aug 9 15:40:03 2016
>> @@ -1892,8 +1892,18 @@ unsigned SCEVExpander::replaceCongruentI
>> return NumElim;
>> }
>>
>> -Value *SCEVExpander::findExistingExpansion(const SCEV *S,
>> - const Instruction *At, Loop *L) {
>> +Value *SCEVExpander::getExactExistingExpansion(const SCEV *S,
>> + const Instruction *At, Loop *L) {
>> + Optional<ScalarEvolution::ValueOffsetPair> VO =
>> + getRelatedExistingExpansion(S, At, L);
>> + if (VO && VO.getValue().second == nullptr)
>> + return VO.getValue().first;
>> + return nullptr;
>> +}
>> +
>> +Optional<ScalarEvolution::ValueOffsetPair>
>> +SCEVExpander::getRelatedExistingExpansion(const SCEV *S, const Instruction *At,
>> + Loop *L) {
>> using namespace llvm::PatternMatch;
>>
>> SmallVector<BasicBlock *, 4> ExitingBlocks;
>> @@ -1911,22 +1921,23 @@ Value *SCEVExpander::findExistingExpansi
>> continue;
>>
>> if (SE.getSCEV(LHS) == S && SE.DT.dominates(LHS, At))
>> - return LHS;
>> + return ScalarEvolution::ValueOffsetPair(LHS, nullptr);
>>
>> if (SE.getSCEV(RHS) == S && SE.DT.dominates(RHS, At))
>> - return RHS;
>> + return ScalarEvolution::ValueOffsetPair(RHS, nullptr);
>> }
>>
>> // Use expand's logic which is used for reusing a previous Value in
>> // ExprValueMap.
>> - if (Value *Val = FindValueInExprValueMap(S, At).first)
>> - return Val;
>> + ScalarEvolution::ValueOffsetPair VO = FindValueInExprValueMap(S, At);
>> + if (VO.first)
>> + return VO;
>>
>> // There is potential to make this significantly smarter, but this simple
>> // heuristic already gets some interesting cases.
>>
>> // Can not find suitable value.
>> - return nullptr;
>> + return None;
>> }
>>
>> bool SCEVExpander::isHighCostExpansionHelper(
>> @@ -1935,7 +1946,7 @@ bool SCEVExpander::isHighCostExpansionHe
>>
>> // If we can find an existing value for this scev avaliable at the point "At"
>> // then consider the expression cheap.
>> - if (At && findExistingExpansion(S, At, L) != nullptr)
>> + if (At && getRelatedExistingExpansion(S, At, L))
>> return false;
>>
>> // Zero/One operand expressions
>> @@ -1983,7 +1994,7 @@ bool SCEVExpander::isHighCostExpansionHe
>> // involving division. This is just a simple search heuristic.
>> if (!At)
>> At = &ExitingBB->back();
>> - if (!findExistingExpansion(
>> + if (!getRelatedExistingExpansion(
>> SE.getAddExpr(S, SE.getConstant(S->getType(), 1)), At, L))
>> return true;
>> }
>>
>> Modified: llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp?rev=278161&r1=278160&r2=278161&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp Tue Aug 9 15:40:03 2016
>> @@ -481,7 +481,7 @@ Value *IndVarSimplify::expandSCEVIfNeede
>> Type *ResultTy) {
>> // Before expanding S into an expensive LLVM expression, see if we can use an
>> // already existing value as the expansion for S.
>> - if (Value *ExistingValue = Rewriter.findExistingExpansion(S, InsertPt, L))
>> + if (Value *ExistingValue = Rewriter.getExactExistingExpansion(S, InsertPt, L))
>> if (ExistingValue->getType() == ResultTy)
>> return ExistingValue;
>>
>>
>> Added: llvm/trunk/test/Analysis/ScalarEvolution/pr28705.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ScalarEvolution/pr28705.ll?rev=278161&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/Analysis/ScalarEvolution/pr28705.ll (added)
>> +++ llvm/trunk/test/Analysis/ScalarEvolution/pr28705.ll Tue Aug 9 15:40:03 2016
>> @@ -0,0 +1,41 @@
>> +; PR28705
>> +; RUN: opt < %s -indvars -S | FileCheck %s
>> +
>> +; Check IndVarSimplify replaces the exitval use of the induction var "%inc.i.i"
>> +; with "%.sroa.speculated + 1".
>> +;
>> +; CHECK-LABEL: @foo(
>> +; CHECK: %[[EXIT:.+]] = sub i32 %.sroa.speculated, -1
>> +; CHECK: %DB.sroa.9.0.lcssa = phi i32 [ 1, %entry ], [ %[[EXIT]], %loopexit ]
>> +;
>> +define void @foo(i32 %sub.ptr.div.i, i8* %ref.i1174) local_unnamed_addr {
>> +entry:
>> + %cmp.i1137 = icmp ugt i32 %sub.ptr.div.i, 3
>> + %.sroa.speculated = select i1 %cmp.i1137, i32 3, i32 %sub.ptr.div.i
>> + %cmp6483126 = icmp eq i32 %.sroa.speculated, 0
>> + br i1 %cmp6483126, label %XZ.exit, label %for.body650.lr.ph
>> +
>> +for.body650.lr.ph:
>> + br label %for.body650
>> +
>> +loopexit:
>> + %inc.i.i.lcssa = phi i32 [ %inc.i.i, %for.body650 ]
>> + br label %XZ.exit
>> +
>> +XZ.exit:
>> + %DB.sroa.9.0.lcssa = phi i32 [ 1, %entry ], [ %inc.i.i.lcssa, %loopexit ]
>> + br label %end
>> +
>> +for.body650:
>> + %iv = phi i32 [ 0, %for.body650.lr.ph ], [ %inc655, %for.body650 ]
>> + %iv2 = phi i32 [ 1, %for.body650.lr.ph ], [ %inc.i.i, %for.body650 ]
>> + %arrayidx.i.i1105 = getelementptr inbounds i8, i8* %ref.i1174, i32 %iv2
>> + store i8 7, i8* %arrayidx.i.i1105, align 1
>> + %inc.i.i = add i32 %iv2, 1
>> + %inc655 = add i32 %iv, 1
>> + %cmp648 = icmp eq i32 %inc655, %.sroa.speculated
>> + br i1 %cmp648, label %loopexit, label %for.body650
>> +
>> +end:
>> + ret void
>> +}
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list