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

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,

-- 
Arnaud A. de Grandmaison