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

[Andrea Di Biagio]

Committed revision 217950.
Thanks Duncan!

On Wed, Sep 17, 2014 at 12:03 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> 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!
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits