[PATCH] InstCombine: constant comparison involving ashr is wrongly simplified (PR20945).

[Andrea Di Biagio]

> I don't think that case exists.  `lshr` deals with unsigned numbers.
> There's no such thing as a negative unsigned number.  For `lshr`, it's
> easier to think about numbers as bitstrings.
>
>> Otherwise, it would have failed for the following case:
>>
>> {code}
>> define i1 @foo(i8 %a) {
>>  %shr = lshr exact i8 -128, %a
>>  %cmp = icmp eq i8 %shr, 1
>>  ret i1 %cmp
>> }
>> {code}
>
> In binary, this translates to:
>
>     define i1 @foo(i8 %a) {
>       %shr = lshr exact i8 0b10000000, %a
>       %cmp = icmp eq i8 %shr, 0b00000001
>       ret i1 %cmp
>     }
>
> AFAICT, this transformation should produce:
>
>     define i1 @foo(i8 %a) {
>       %cmp = icmp eq i8 %a, 7
>       ret i1 %cmp
>     }
>
> Does it do something else?  What?  Can you add this as a testcase?

Sorry, you are right.
That test does exactly what you said (i.e. it produces the compare between %a and 7).
Btw, that is from test 'icmp-shr.ll'. It is function @exact_lshr_eq_opposite_msb.
I think I was probably using the wrong compiler when I saw that test case failing.. sorry for the noise.

>
>>
>> Please let me know what you think.
>> Thanks,
>> Andrea
>>
>> http://reviews.llvm.org/D5356
>>
>> Files:
>>  lib/Transforms/InstCombine/InstCombineCompares.cpp
>>  test/Transforms/InstCombine/icmp-shr.ll
>> <D5356.13759.patch>
>
>>
>> +  if (!IsAShr) {
>> +    bool IsAP1Negative = AP1.isNegative();
>> +    bool IsAP2Negative = AP2.isNegative();
>> +    if (IsAP1Negative && IsAP2Negative)
>> +      // Logical shift by a non-zero quantity always changes the sign bit of a
>> +      // negative number.
>> +      return getConstant(false);
>> +    if (IsAP1Negative)
>> +      // Logical shift never affects the sign of a positive number.
>> +      return getConstant(false);
>> +    if (!IsAP2Negative && AP1.ugt(AP2))
>> +      // Right-shifting will not increase the value.
>> +      return getConstant(false);
>> +  }
>
> This new logic doesn't make sense to me.  Note that `isNegative()` just
> tells you whether the MSB is set to 1.  How is the MSB relevant here?
> (It certainly isn't a sign bit.)

I agree it is not relevant.
Again, sorry for the confusion.

Here is an updated patch.
-Andrea

http://reviews.llvm.org/D5356

Files:
  lib/Transforms/InstCombine/InstCombineCompares.cpp
  test/Transforms/InstCombine/icmp-shr.ll
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D5356.13764.patch
Type: text/x-patch
Size: 2427 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140916/a15e33d2/attachment.bin>