[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:51:55 PDT 2017
After sending this, I found the commit where Sanjoy reverted the change,
so that part of my message is no longer relevant. The rest stands.
Philip
On 10/20/2017 03:50 PM, Philip Reames wrote:
>
> 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/062ea813/attachment.html>
More information about the llvm-commits
mailing list