[llvm] r278161 - Fix the runtime error caused by "Use ValueOffsetPair to enhance value reuse during SCEV expansion".
Steven Wu via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 13 12:01:42 PDT 2016
ping. Is there any update on this?
Steven
> On Aug 29, 2016, at 11:19 AM, Wei Mi <wmi at google.com> wrote:
>
> 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