[PATCH] Teach Instcombine to use the nsw attribute for signed comparisons to 0 of (shl %v, Cst) or (mul %v, Cst)

[Nick Lewycky]

+static bool isSignTest(ICmpInst::Predicate& pred, const ConstantInt *RHS) {
+  if (!ICmpInst::isSigned(pred))
+    return 0;

Please return true/false when returning a bool. Also, "& pred" should be "
&pred".

+      if (isSignTest(pred, RHS) && Val->getSExtValue() != 0 &&

Val->getSExtValue() will assert if Val has more than 64 bits. Did you mean
"!Val->isZero()"?

+      if (cast<BinaryOperator>(LHSI)->hasNoSignedWrap() && RHSV==0)

Missing spaces around "==".

+    if (isSignTest(pred, RHS) &&
+        cast<BinaryOperator>(LHSI)->hasNoSignedWrap())
+        return new ICmpInst(pred,
+                            LHSI->getOperand(0),
+                            Constant::getNullValue(RHS->getType()));

Incorrect indentation, the 'return' should be two spaces in from the 'if'.

+        if (RHSV==0) {

Again missing spaces around "==".

+            if (BOC->getValue() == 0)

That works, but BOC->isZero() uses trickery to be more efficient for large
integers.

+      case Instruction::Mul:
+        if (RHSV==0) {
+          if (ConstantInt *BOC = dyn_cast<ConstantInt>(BO->getOperand(1)))
{
+            // Trivial case : if we have (mul X, 0), then the comparison
to 0
+            // always fail or succeed, depending on the predicate
+            if (BOC->getValue() == 0)
+              return ReplaceInstUsesWith(ICI,
+
ConstantInt::get(Type::getInt1Ty(ICI.getContext()),
+                                         isICMP_NE));

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 ).


Thanks for the testcases. Feel free to commit after resolving the above
comments.

Nick



On 22 March 2013 15:43, Arnaud A. de Grandmaison <arnaud.adegm at gmail.com>wrote:

> Ping ?
>
> On Wednesday 20 March 2013 22:46:17 Arnaud de Grandmaison wrote:
> > Ping ?
> >
> > On Monday 18 March 2013 14:54:32 Arnaud A. de Grandmaison wrote:
> > > On 03/18/2013 12:19 PM, Arnaud A. de Grandmaison wrote:
> > > > On 03/18/2013 07:07 AM, Nick Lewycky wrote:
> > > >> Arnaud de Grandmaison wrote:
> > > >>> The attached patch teaches
> InstCombine/visitICmpInstWithInstAndIntCst
> > > >>> to
> > > >>> optimize 'icmp pred (mul %v, Cst), 0' when the mul has the nsw
> > > >>> attribute set.
> > > >>>
> > > >>>   The comparison can the be performed on the mul RHS.
> > > >>>
> > > >>> The patch also handles the case of the mul in strength reduced
> form :
> > > >>> shl
> > > >>
> > > >> There's two optimizations here: remove operations that don't change
> > > >> sign test and only feed into a sign test, and remove operations that
> > > >> don't change "zeroness" and only feed into an is-zero test.
> > > >
> > > > My patch is only about the former optimization, but I can (and will)
> add
> > > > the second optimization.
> > > >
> > > >> We already have the latter, so please extend it. You can find it
> > > >> starting at InstCombineCompares.cpp:1419, and it supports all sorts
> of
> > > >> great cases like "popcount(A) == 0  ->  A == 0" (line 1555).
> > > >>
> > > >> Then append the sign testing bits to that function
> > > >> (visitICmpInstWithInstAndIntCst). Your "isCompareToZero" becomes
> > > >> "isSignTest".
> > > >
> > > > This is how I first named the function, but I disliked it as it
> conveys
> > > > the idea that it is about sign bit check. Will rename it to
> > > > "isSignTest".
> > > >
> > > >> The tests only cover multiply, but the code checks for 'nsw' on any
> > > >> BinaryOperator. Are we already doing this correctly for 'and' and
> > > >> 'sub', and if so, through what code path (and why not extend that)?
> > > >
> > > > I do not follow you there : the nsw check and optimization is only
> done
> > > > for mul and shl. Other operators are not handled at the moment.
> > > >
> > > > My appproach was to implement it correctly for the mul/shl case and
> ease
> > > > the review by moving forward in steps with other operators. The
> mul/shl
> > > > is just the first step. Other steps will be done in other patches.
> The
> > > > only reason I do mul & shl at the same time is that shl is often the
> > > > strenght reduced form of mul.
> > > >
> > > >> What if the instruction is an 'or' whose RHS includes the sign bit?
> Or
> > > >> an 'xor' that doesn't have the sign bit in the RHS? Were we already
> > > >> getting these? Through SimplifyDemandedBits?
> > > >>
> > > >> If not, please extend the tests. If so, you may want to remove any
> > > >> optimizations that are now redundant?
> > > >
> > > > Other operators (or, xor, ..) are not handled by this patch.
> > > >
> > > >> Nick
> > > >
> > > > Thanks for your  time and review.
> > > >
> > > > I am reworking the patch and will post it soon.
> > > >
> > > > Cheers,
> > >
> > > Hi Nick and Duncan,
> > >
> > > Here is the second take for the patch, where I believe all your points
> > > have been addressed.
> > >
> > > For the shl optimizations, the code is not split up in 2 places (one
> > > place for signedness related optimization, the other place for zeroness
> > > related optimizations) because I am reusing the existing control flow.
> > > Shall I split this in 2 ? My guess is this was not done originally to
> > > avoid some code duplication.
> > >
> > > Cheers,
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130322/840ffdd6/attachment.html>