[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]

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,