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

[Duncan P. N. Exon Smith]

> On 2014-Sep-16, at 12:58, Andrea Di Biagio <Andrea_DiBiagio at sn.scee.net> wrote:
> 
> Hi Duncan,
> 
> Here is an updated patch which I think/hope addresses all your comments.
> 
> There is one main difference with respect to what you suggested (see below)
> 
>> Then change this to:
>> 
>>    if (!IsNegative && AP1.ugt(AP2))
>>      // Right-shifting won't increase the magnitude.
>>      return getConstant(false);
>> 
> 
> That check should also take into account the case where 'IsAShr' is false, AP2 is negative and AP1 is positive.

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?

> 
> 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.)