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

[Duncan P. N. Exon Smith]

> On 2014-Sep-16, at 14:58, Andrea Di Biagio <Andrea_DiBiagio at sn.scee.net> wrote:
> 
>> 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
> <D5356.13764.patch>

LGTM.  Thanks again for fixing this!