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

-- 
Arnaud A. de Grandmaison

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-InstCombine-simplify-comparisons-to-zero-of-shl-x-Cs.patch
Type: text/x-patch
Size: 9107 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130318/78f7149b/attachment.bin>