<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Committed as r177856.<br>
<br>
InstSimplify was already handling the trivial (mul X, 0) case, so
there was nothing to do there.<br>
<br>
Thanks for the review.<br>
<br>
Cheers,<br>
--<br>
Arnaud<br>
<br>
On 03/23/2013 01:08 AM, Nick Lewycky wrote:<br>
</div>
<blockquote
cite="mid:CADbEz-jO0AiHLmcTw3zXLCwiiw4pvTWA4j+R8ane_JLEMJfxyw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>+static bool isSignTest(ICmpInst::Predicate& pred,
const ConstantInt *RHS) {</div>
<div>+ if (!ICmpInst::isSigned(pred))</div>
<div>+ return 0;</div>
<div><br>
</div>
<div style="">Please return true/false when returning a bool.
Also, "& pred" should be " &pred".</div>
<div style=""><br>
</div>
<div style="">+ if (isSignTest(pred, RHS) &&
Val->getSExtValue() != 0 &&<br>
</div>
<div style=""><br>
</div>
<div style="">Val->getSExtValue() will assert if Val has more
than 64 bits. Did you mean "!Val->isZero()"?</div>
<div style=""><br>
</div>
<div style="">+ if
(cast<BinaryOperator>(LHSI)->hasNoSignedWrap()
&& RHSV==0)<br>
</div>
<div style=""><br>
</div>
<div style="">Missing spaces around "==".</div>
<div style=""><br>
</div>
<div style="">
<div>+ if (isSignTest(pred, RHS) &&</div>
<div>+
cast<BinaryOperator>(LHSI)->hasNoSignedWrap())</div>
<div>+ return new ICmpInst(pred,</div>
<div>+ LHSI->getOperand(0),</div>
<div>+
Constant::getNullValue(RHS->getType()));</div>
<div><br>
</div>
<div style="">Incorrect indentation, the 'return' should be
two spaces in from the 'if'.</div>
<div style=""><br>
</div>
<div style="">+ if (RHSV==0) {<br>
</div>
<div style=""><br>
</div>
<div style="">Again missing spaces around "==".</div>
</div>
<div style=""><br>
</div>
<div style="">+ if (BOC->getValue() == 0)<br>
</div>
<div style="">
<br>
</div>
<div style="">That works, but BOC->isZero() uses trickery to
be more efficient for large integers.</div>
<div style=""><br>
</div>
<div style="">+ case Instruction::Mul:<br>
</div>
<div style="">
<div>+ if (RHSV==0) {</div>
<div>+ if (ConstantInt *BOC =
dyn_cast<ConstantInt>(BO->getOperand(1))) {</div>
<div>+ // Trivial case : if we have (mul X, 0),
then the comparison to 0</div>
<div>+ // always fail or succeed, depending on the
predicate</div>
<div>+ if (BOC->getValue() == 0)</div>
<div>+ return ReplaceInstUsesWith(ICI,</div>
<div>+
ConstantInt::get(Type::getInt1Ty(ICI.getContext()),</div>
<div>+ isICMP_NE));</div>
<div><br>
</div>
<div style="">Instead, can you make InstSimplify handle this
case? InstSimplify is intended to handle the cases where we
don't need to form a new instruction, so for instance where
the result is a constant or is a subset of the input ( %a =
%x+1 / %b = %a-1 --> %a ).</div>
<div><br>
</div>
<div><br>
</div>
</div>
<div style="">Thanks for the testcases. Feel free to commit
after resolving the above comments.</div>
<div style=""><br>
</div>
<div style="">Nick</div>
<div><br>
</div>
</div>
<div class="gmail_extra">
<br>
<br>
<div class="gmail_quote">On 22 March 2013 15:43, Arnaud A. de
Grandmaison <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:arnaud.adegm@gmail.com" target="_blank">arnaud.adegm@gmail.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
Ping ?<br>
<div class="HOEnZb">
<div class="h5"><br>
On Wednesday 20 March 2013 22:46:17 Arnaud de
Grandmaison wrote:<br>
> Ping ?<br>
><br>
> On Monday 18 March 2013 14:54:32 Arnaud A. de
Grandmaison wrote:<br>
> > On 03/18/2013 12:19 PM, Arnaud A. de
Grandmaison wrote:<br>
> > > On 03/18/2013 07:07 AM, Nick Lewycky
wrote:<br>
> > >> Arnaud de Grandmaison wrote:<br>
> > >>> The attached patch teaches
InstCombine/visitICmpInstWithInstAndIntCst<br>
> > >>> to<br>
> > >>> optimize 'icmp pred (mul %v,
Cst), 0' when the mul has the nsw<br>
> > >>> attribute set.<br>
> > >>><br>
> > >>> The comparison can the be
performed on the mul RHS.<br>
> > >>><br>
> > >>> The patch also handles the case
of the mul in strength reduced form :<br>
> > >>> shl<br>
> > >><br>
> > >> There's two optimizations here:
remove operations that don't change<br>
> > >> sign test and only feed into a sign
test, and remove operations that<br>
> > >> don't change "zeroness" and only feed
into an is-zero test.<br>
> > ><br>
> > > My patch is only about the former
optimization, but I can (and will) add<br>
> > > the second optimization.<br>
> > ><br>
> > >> We already have the latter, so please
extend it. You can find it<br>
> > >> starting at
InstCombineCompares.cpp:1419, and it supports all sorts
of<br>
> > >> great cases like "popcount(A) == 0
-> A == 0" (line 1555).<br>
> > >><br>
> > >> Then append the sign testing bits to
that function<br>
> > >> (visitICmpInstWithInstAndIntCst).
Your "isCompareToZero" becomes<br>
> > >> "isSignTest".<br>
> > ><br>
> > > This is how I first named the function,
but I disliked it as it conveys<br>
> > > the idea that it is about sign bit check.
Will rename it to<br>
> > > "isSignTest".<br>
> > ><br>
> > >> The tests only cover multiply, but
the code checks for 'nsw' on any<br>
> > >> BinaryOperator. Are we already doing
this correctly for 'and' and<br>
> > >> 'sub', and if so, through what code
path (and why not extend that)?<br>
> > ><br>
> > > I do not follow you there : the nsw check
and optimization is only done<br>
> > > for mul and shl. Other operators are not
handled at the moment.<br>
> > ><br>
> > > My appproach was to implement it
correctly for the mul/shl case and ease<br>
> > > the review by moving forward in steps
with other operators. The mul/shl<br>
> > > is just the first step. Other steps will
be done in other patches. The<br>
> > > only reason I do mul & shl at the
same time is that shl is often the<br>
> > > strenght reduced form of mul.<br>
> > ><br>
> > >> What if the instruction is an 'or'
whose RHS includes the sign bit? Or<br>
> > >> an 'xor' that doesn't have the sign
bit in the RHS? Were we already<br>
> > >> getting these? Through
SimplifyDemandedBits?<br>
> > >><br>
> > >> If not, please extend the tests. If
so, you may want to remove any<br>
> > >> optimizations that are now redundant?<br>
> > ><br>
> > > Other operators (or, xor, ..) are not
handled by this patch.<br>
> > ><br>
> > >> Nick<br>
> > ><br>
> > > Thanks for your time and review.<br>
> > ><br>
> > > I am reworking the patch and will post it
soon.<br>
> > ><br>
> > > Cheers,<br>
> ><br>
> > Hi Nick and Duncan,<br>
> ><br>
> > Here is the second take for the patch, where I
believe all your points<br>
> > have been addressed.<br>
> ><br>
> > For the shl optimizations, the code is not
split up in 2 places (one<br>
> > place for signedness related optimization, the
other place for zeroness<br>
> > related optimizations) because I am reusing
the existing control flow.<br>
> > Shall I split this in 2 ? My guess is this was
not done originally to<br>
> > avoid some code duplication.<br>
> ><br>
> > Cheers,<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a moz-do-not-send="true"
href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a moz-do-not-send="true"
href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits"
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Arnaud A. de Grandmaison</pre>
</body>
</html>