[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:06:20 PDT 2016
Ahh, sorry. I did see that review. Please ignore the noise. Thanks for looking at it!
Steven
> On Sep 13, 2016, at 12:04 PM, Wei Mi <wmi at google.com> wrote:
>
> Hi Steven,
>
> The patch is still under review. I am trying to construct a C++
> testcase to test a path in the patch. I hope the patch can be
> committed in one or two days.
>
> https://reviews.llvm.org/D24088
>
> Thanks,
> Wei.
>
> On Tue, Sep 13, 2016 at 12:01 PM, Steven Wu <stevenwu at apple.com> wrote:
>> 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