<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 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 href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a 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>