[llvm] r316054 - [ScalarEvolution] Handling for ICmp occuring in the evolution chain.
Sanjoy Das via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 18 10:04:47 PDT 2017
Hi Jatin,
On Tue, Oct 17, 2017 at 11:28 PM, Jatin Bhateja <jatin.bhateja at gmail.com> wrote:
> Hi Sanjoy,
>
> If its against policy then there is no question of not reverting.
>
> However, since its already made its way to trunk and regressed and checkin
> was done after no open comments. Can we have post checkin comments if that
> sounds reasonable to you.
I don't think this patch in particular is problematic, but I'm worried
about setting a bad precedent. I'm worried that if we do this now, we
will have a harder time pushing back on future instances where the
committer may not be as accommodating as you are and/or where the
patch itself may be much more problematic. I hope you understand.
As for the review, I'm a somewhat busy today and tomorrow with the
llvm developers' meeting, but I will make sure I find time to review
it by the end of this week.
-- Sanjoy
>
> Thanks,
> Jatin
>
> On Wednesday, October 18, 2017, Davide Italiano <davide at freebsd.org> wrote:
>>
>> I'm afraid this is not really reasonable.
>> I think we shouldn't really land until all comments are addressed. On
>> top of this, Sanjoy owns SCEV so he has the final word.
>> You should consider reverting this patch.
>>
>> On Tue, Oct 17, 2017 at 10:19 PM, Jatin Bhateja via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>> >
>> > It was accepted by one reviewer which is why it was landed as per
>> > policy.
>> >
>> > We can do a post commit review also as per developer's policy.
>> >
>> > Do let me know what comments you have and it could be incorporated.
>> >
>> >
>> > On 18 Oct 2017 10:22, "Sanjoy Das" <sanjoy at playingwithpointers.com>
>> > wrote:
>> >>
>> >> Hi Jatin,
>> >>
>> >> This hasn't been LGTMed. Please revert it for now while we continue the
>> >> review.
>> >>
>> >> -- Sanjoy
>> >>
>> >> On Tue, Oct 17, 2017 at 6:36 PM Jatin Bhateja via llvm-commits
>> >> <llvm-commits at lists.llvm.org> wrote:
>> >>>
>> >>> Author: jbhateja
>> >>> Date: Tue Oct 17 18:36:16 2017
>> >>> New Revision: 316054
>> >>>
>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=316054&view=rev
>> >>> Log:
>> >>> [ScalarEvolution] Handling for ICmp occuring in the evolution chain.
>> >>>
>> >>> Summary:
>> >>> If a compare instruction is same or inverse of the compare in the
>> >>> branch of the loop latch, then return a constant evolution node.
>> >>> Currently scope of evaluation is limited to SCEV computation for
>> >>> PHI nodes.
>> >>>
>> >>> This shall facilitate computations of loop exit counts in cases
>> >>> where compare appears in the evolution chain of induction variables.
>> >>>
>> >>> Will fix PR 34538
>> >>> Reviewers: sanjoy, hfinkel, junryoungju
>> >>>
>> >>> Reviewed By: junryoungju
>> >>>
>> >>> Subscribers: javed.absar, llvm-commits
>> >>>
>> >>> Differential Revision: https://reviews.llvm.org/D38494
>> >>>
>> >>> Added:
>> >>> llvm/trunk/test/Analysis/ScalarEvolution/pr34538.ll
>> >>> Modified:
>> >>> llvm/trunk/include/llvm/Analysis/ScalarEvolution.h
>> >>> llvm/trunk/lib/Analysis/ScalarEvolution.cpp
>> >>> llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp
>> >>>
>> >>> Modified: llvm/trunk/include/llvm/Analysis/ScalarEvolution.h
>> >>> URL:
>> >>>
>> >>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/ScalarEvolution.h?rev=316054&r1=316053&r2=316054&view=diff
>> >>>
>> >>>
>> >>> ==============================================================================
>> >>> --- llvm/trunk/include/llvm/Analysis/ScalarEvolution.h (original)
>> >>> +++ llvm/trunk/include/llvm/Analysis/ScalarEvolution.h Tue Oct 17
>> >>> 18:36:16 2017
>> >>> @@ -1378,6 +1378,9 @@ private:
>> >>> /// Helper function called from createNodeForPHI.
>> >>> const SCEV *createAddRecFromPHI(PHINode *PN);
>> >>>
>> >>> + /// Evaluate ICmpInst to a constant node for special patterns.
>> >>> + const SCEV *evaluateForICmp(ICmpInst *IC);
>> >>> +
>> >>> /// A helper function for createAddRecFromPHI to handle simple
>> >>> cases.
>> >>> const SCEV *createSimpleAffineAddRec(PHINode *PN, Value *BEValueV,
>> >>> Value *StartValueV);
>> >>>
>> >>> Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
>> >>> URL:
>> >>>
>> >>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=316054&r1=316053&r2=316054&view=diff
>> >>>
>> >>>
>> >>> ==============================================================================
>> >>> --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
>> >>> +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Tue Oct 17 18:36:16
>> >>> 2017
>> >>> @@ -4756,11 +4756,26 @@ const SCEV *ScalarEvolution::createAddRe
>> >>> Ops.push_back(Add->getOperand(i));
>> >>> const SCEV *Accum = getAddExpr(Ops);
>> >>>
>> >>> + bool InvariantF = isLoopInvariant(Accum, L);
>> >>> +
>> >>> + if (!InvariantF && Accum->getSCEVType() == scZeroExtend) {
>> >>> + const SCEV *Op =
>> >>> dyn_cast<SCEVZeroExtendExpr>(Accum)->getOperand();
>> >>> + const SCEVUnknown *Un = dyn_cast<SCEVUnknown>(Op);
>> >>> + if (Un && Un->getValue() && isa<Instruction>(Un->getValue())
>> >>> &&
>> >>> + dyn_cast<Instruction>(Un->getValue())->getOpcode() ==
>> >>> + Instruction::ICmp) {
>> >>> + const SCEV *ICmpSC =
>> >>> evaluateForICmp(cast<ICmpInst>(Un->getValue()));
>> >>> + bool IsConstSC = ICmpSC->getSCEVType() == scConstant;
>> >>> + Accum =
>> >>> + IsConstSC ? getZeroExtendExpr(ICmpSC, Accum->getType())
>> >>> :
>> >>> Accum;
>> >>> + InvariantF = IsConstSC ? true : false;
>> >>> + }
>> >>> + }
>> >>> +
>> >>> // This is not a valid addrec if the step amount is varying
>> >>> each
>> >>> // loop iteration, but is not itself an addrec in this loop.
>> >>> - if (isLoopInvariant(Accum, L) ||
>> >>> - (isa<SCEVAddRecExpr>(Accum) &&
>> >>> - cast<SCEVAddRecExpr>(Accum)->getLoop() == L)) {
>> >>> + if (InvariantF || (isa<SCEVAddRecExpr>(Accum) &&
>> >>> + cast<SCEVAddRecExpr>(Accum)->getLoop() ==
>> >>> L)) {
>> >>> SCEV::NoWrapFlags Flags = SCEV::FlagAnyWrap;
>> >>>
>> >>> if (auto BO = MatchBinaryOp(BEValueV, DT)) {
>> >>> @@ -6443,6 +6458,30 @@ void ScalarEvolution::forgetLoop(const L
>> >>> }
>> >>> }
>> >>>
>> >>> +
>> >>> +const SCEV *ScalarEvolution::evaluateForICmp(ICmpInst *IC) {
>> >>> + BasicBlock *Latch = nullptr;
>> >>> + const Loop *L = LI.getLoopFor(IC->getParent());
>> >>> +
>> >>> + // If compare instruction is same or inverse of the compare in the
>> >>> + // branch of the loop latch, then return a constant evolution
>> >>> + // node. This shall facilitate computations of loop exit counts
>> >>> + // in cases where compare appears in the evolution chain of
>> >>> induction
>> >>> + // variables.
>> >>> + if (L && (Latch = L->getLoopLatch())) {
>> >>> + BranchInst *BI = dyn_cast<BranchInst>(Latch->getTerminator());
>> >>> + if (BI && BI->isConditional() && BI->getCondition() == IC) {
>> >>> + if (BI->getSuccessor(0) != L->getHeader())
>> >>> + return getZero(Type::getInt1Ty(getContext()));
>> >>> + else
>> >>> + return getOne(Type::getInt1Ty(getContext()));
>> >>> + }
>> >>> + }
>> >>> +
>> >>> + return getUnknown(IC);
>> >>> +}
>> >>> +
>> >>> +
>> >>> void ScalarEvolution::forgetValue(Value *V) {
>> >>> Instruction *I = dyn_cast<Instruction>(V);
>> >>> if (!I) return;
>> >>>
>> >>> Modified: llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp
>> >>> URL:
>> >>>
>> >>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp?rev=316054&r1=316053&r2=316054&view=diff
>> >>>
>> >>>
>> >>> ==============================================================================
>> >>> --- llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp (original)
>> >>> +++ llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp Tue Oct 17
>> >>> 18:36:16 2017
>> >>> @@ -2973,8 +2973,11 @@ void LSRInstance::CollectChains() {
>> >>> // Ignore users that are part of a SCEV expression. This way we
>> >>> only
>> >>> // consider leaf IV Users. This effectively rediscovers a
>> >>> portion
>> >>> of
>> >>> // IVUsers analysis but in program order this time.
>> >>> - if (SE.isSCEVable(I.getType()) &&
>> >>> !isa<SCEVUnknown>(SE.getSCEV(&I)))
>> >>> - continue;
>> >>> + if (SE.isSCEVable(I.getType())) {
>> >>> + const SCEV *SI = SE.getSCEV(&I);
>> >>> + if (!isa<SCEVUnknown>(SI) && !isa<SCEVConstant>(SI))
>> >>> + continue;
>> >>> + }
>> >>>
>> >>> // Remove this instruction from any NearUsers set it may be in.
>> >>> for (unsigned ChainIdx = 0, NChains = IVChainVec.size();
>> >>>
>> >>> Added: llvm/trunk/test/Analysis/ScalarEvolution/pr34538.ll
>> >>> URL:
>> >>>
>> >>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ScalarEvolution/pr34538.ll?rev=316054&view=auto
>> >>>
>> >>>
>> >>> ==============================================================================
>> >>> --- llvm/trunk/test/Analysis/ScalarEvolution/pr34538.ll (added)
>> >>> +++ llvm/trunk/test/Analysis/ScalarEvolution/pr34538.ll Tue Oct 17
>> >>> 18:36:16 2017
>> >>> @@ -0,0 +1,19 @@
>> >>> +; RUN: opt -S -scalar-evolution -loop-deletion -simplifycfg -analyze
>> >>> <
>> >>> %s | FileCheck %s --check-prefix=CHECK-ANALYSIS
>> >>> +
>> >>> +define i32 @foo() local_unnamed_addr #0 {
>> >>> +; CHECK-ANALYSIS: Loop %do.body: backedge-taken count is 10000
>> >>> +; CHECK-ANALYSIS: Loop %do.body: max backedge-taken count is 10000
>> >>> +; CHECK-ANALYSIS: Loop %do.body: Predicated backedge-taken count is
>> >>> 10000
>> >>> +entry:
>> >>> + br label %do.body
>> >>> +
>> >>> +do.body: ; preds = %do.body,
>> >>> %entry
>> >>> + %start.0 = phi i32 [ 0, %entry ], [ %inc.start.0, %do.body ]
>> >>> + %cmp = icmp slt i32 %start.0, 10000
>> >>> + %inc = zext i1 %cmp to i32
>> >>> + %inc.start.0 = add nsw i32 %start.0, %inc
>> >>> + br i1 %cmp, label %do.body, label %do.end
>> >>> +
>> >>> +do.end: ; preds = %do.body
>> >>> + ret i32 0
>> >>> +}
>> >>>
>> >>>
>> >>> _______________________________________________
>> >>> 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
>> >
>>
>> --
>> Davide
>>
>> "There are no solved problems; there are only problems that are more
>> or less solved" -- Henri Poincare
>
>
>
> --
> Jatin Bhateja
More information about the llvm-commits
mailing list