[llvm] r316054 - [ScalarEvolution] Handling for ICmp occuring in the evolution chain.
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 20 15:50:55 PDT 2017
Jatin,
I feel you are missing an important point in this discussion.
*You* have been asked to revert the patch. Sanjoy is the code owner for
this component, but *any* LLVM developer can ask you to revert a patch.
When asked, you are expected revert the patch not defer that work to
anyone else. In particular, discussion of the merits of the revert are
expected to come *after* the revert is done. We have a strongly
enforced revert-to-green policy and complying with that is a key element
of being an LLVM contributor.
Please revert the patch. Reply to this thread with the change set in
which you did so.
After that is done, discussion on the merits of the patch can continue
on the review thread.
Philip
On 10/18/2017 12:10 PM, Jatin Bhateja via llvm-commits wrote:
> 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
> <mailto: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
> <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
> <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
> <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
> <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
> <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
> <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
> <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
> <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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171020/79725dbd/attachment.html>
More information about the llvm-commits
mailing list