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