[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
Tue Sep 13 12:04:46 PDT 2016
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