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

[Arnaud de Grandmaison]

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,