<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Jatin,</p>
<p>I feel you are missing an important point in this discussion.</p>
<p>*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.</p>
<p>Please revert the patch. Reply to this thread with the change
set in which you did so.</p>
<p>After that is done, discussion on the merits of the patch can
continue on the review thread.<br>
</p>
<p>Philip<br>
</p>
<br>
<div class="moz-cite-prefix">On 10/18/2017 12:10 PM, Jatin Bhateja
via llvm-commits wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CABMDQ41pM6HcuaS8BK-qRFAory5WZwwg8uYbCEQNk0BDJAms-A@mail.gmail.com">
<div>Hi Sanjoy,</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Thanks</div>
<div><br>
</div>
<div><br>
</div>
<div><span style="font-size:15px"><br>
</span>On Wednesday, October 18, 2017, Sanjoy Das <<a
href="mailto:sanjoy@playingwithpointers.com"
moz-do-not-send="true">sanjoy@playingwithpointers.com</a>>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Jatin,<br>
<br>
On Tue, Oct 17, 2017 at 11:28 PM, Jatin Bhateja <<a
href="javascript:;" onclick="_e(event, 'cvml',
'jatin.bhateja@gmail.com')" moz-do-not-send="true">jatin.bhateja@gmail.com</a>>
wrote:<br>
> Hi Sanjoy,<br>
><br>
> If its against policy then there is no question of not
reverting.<br>
><br>
> However, since its already made its way to trunk and
regressed and checkin<br>
> was done after no open comments. Can we have post checkin
comments if that<br>
> sounds reasonable to you.<br>
<br>
I don't think this patch in particular is problematic, but I'm
worried<br>
about setting a bad precedent. I'm worried that if we do this
now, we<br>
will have a harder time pushing back on future instances where
the<br>
committer may not be as accommodating as you are and/or where
the<br>
patch itself may be much more problematic. I hope you
understand.<br>
<br>
As for the review, I'm a somewhat busy today and tomorrow with
the<br>
llvm developers' meeting, but I will make sure I find time to
review<br>
it by the end of this week.<br>
<br>
-- Sanjoy<br>
<br>
><br>
> Thanks,<br>
> Jatin<br>
><br>
> On Wednesday, October 18, 2017, Davide Italiano <<a
href="javascript:;" onclick="_e(event, 'cvml',
'davide@freebsd.org')" moz-do-not-send="true">davide@freebsd.org</a>>
wrote:<br>
>><br>
>> 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')" moz-do-not-send="true">llvm-commits@lists.llvm.org</a>>
wrote:<br>
>> ><br>
>> > It was accepted by one reviewer which is why it
was landed as per<br>
>> > 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')" moz-do-not-send="true">sanjoy@playingwithpointers.com</a>><br>
>> > 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')"
moz-do-not-send="true">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" moz-do-not-send="true">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"
moz-do-not-send="true">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>
>> >>><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" moz-do-not-send="true">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>
>> >>><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<br>
>> >>> 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>
>> >>><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" moz-do-not-send="true">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>
>> >>><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<br>
>> >>> 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>
>> >>> &&<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>
>> >>> :<br>
>> >>> Accum;<br>
>> >>> + InvariantF = IsConstSC ? true
: false;<br>
>> >>> + }<br>
>> >>> + }<br>
>> >>> +<br>
>> >>> // This is not a valid addrec if
the step amount is varying<br>
>> >>> 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() ==<br>
>> >>> 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<br>
>> >>> 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>
>> >>><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" moz-do-not-send="true">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>
>> >>><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<br>
>> >>> 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>
>> >>><br>
>> >>> <a
href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ScalarEvolution/pr34538.ll?rev=316054&view=auto"
target="_blank" moz-do-not-send="true">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Analysis/ScalarEvolution/<wbr>pr34538.ll?rev=316054&view=<wbr>auto</a><br>
>> >>><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>
>> >>> <<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')"
moz-do-not-send="true">llvm-commits@lists.llvm.org</a><br>
>> >>> <a
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
target="_blank" moz-do-not-send="true">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')"
moz-do-not-send="true">llvm-commits@lists.llvm.org</a><br>
>> > <a
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
target="_blank" moz-do-not-send="true">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>
><br>
><br>
><br>
> --<br>
> Jatin Bhateja<br>
</blockquote>
</div>
<br>
<br>
-- <br>
Jatin Bhateja<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</body>
</html>