[PATCH] Teach Instcombine to use the nsw attribute for signed comparisons to 0 of (shl %v, Cst) or (mul %v, Cst)
Arnaud A. de Grandmaison
arnaud.adegm at gmail.com
Mon Mar 25 02:55:26 PDT 2013
Committed as r177856.
InstSimplify was already handling the trivial (mul X, 0) case, so there
was nothing to do there.
Thanks for the review.
Cheers,
--
Arnaud
On 03/23/2013 01:08 AM, Nick Lewycky wrote:
> +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 <mailto: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 <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
--
Arnaud A. de Grandmaison
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130325/a27d2a04/attachment.html>
More information about the llvm-commits
mailing list