[llvm] r316054 - [ScalarEvolution] Handling for ICmp occuring in the evolution chain.
Jatin Bhateja via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 18 12:10:45 PDT 2017
Hi Sanjoy,
I agree, if you see any problems kindly revert as Developer policy empower
code owners to do so. I don't see any issue in it.
Thanks
On Wednesday, October 18, 2017, Sanjoy Das <sanjoy at playingwithpointers.com>
wrote:
> Hi Jatin,
>
> On Tue, Oct 17, 2017 at 11:28 PM, Jatin Bhateja <jatin.bhateja at gmail.com
> <javascript:;>> 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
> <javascript:;>> 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 <javascript:;>> 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
> <javascript:;>>
> >> > 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 <javascript:;>> 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 <javascript:;>
> >> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >> >
> >> >
> >> >
> >> > _______________________________________________
> >> > llvm-commits mailing list
> >> > llvm-commits at lists.llvm.org <javascript:;>
> >> > 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
>
--
Jatin Bhateja
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171019/964617e3/attachment.html>
More information about the llvm-commits
mailing list