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

[Andrea Di Biagio]

Hi Duncan,

Thanks for the feedback.
I have a couple of questions about one of your comments (see below):

> Since the signs aren't being switched, you'll need to change:
>
>     if (AP1.ugt(AP2)) {
>       // Right-shifting will not increase the value.
>       return getConstant(false);
>     }
>
> to:
>
>     if (IsAShr ? AP1.sgt(AP2) : AP1.ugt(AP2)) {
>       // Right-shifting will not increase the value.
>       return getConstant(false);
>     }

Unfortunately this would not work for the case where 'IsNegative &&
IsAShr' is true.
If 'IsNegative' is true, then we have to invert the role of AP1 and
AP2 in the equation.

Something like...
 if (IsAShr) {
   if (IsNegative ? AP2.sgt(AP1) : AP1.sgt(AP2))
     return getConstant(false);
 }

For logical shifts, if both AP1 and AP2 are negative numbers then
(under the assumption that AP1 is different than AP2), we can never
find a 'Shift' value that solves the equation. That is because a
logical shift by a non-zero quantity would affect the sign.

Basically what I am saying is that there is no shift value S for which
the following equation is true:
    -AP1 u>> S == -AP2    ; with S != 0.

So, if you agree, for the case where we have a logical shift we can add:
...
else {
  // we have a logical shift right.
  if (IsNegative || AP1.ugt(AP2))
    return getConstant(false);
}

Note that this would work under the assumption that AP1 and AP2 are
not the same value. For the case where AP1 is equal to AP2, Shift is
trivally zero.

What do you think?

I hope this make sense. In case, I would upload an updated patch.

Thanks,
-Andrea

>
>>
>> @@ -1106,8 +1108,17 @@
>>    // Get the distance between the highest bit that's set.
>>    int Shift = AP2.logBase2() - AP1.logBase2();
>
> You need to update this code to use `IsNegative`:
>
>     int Shift;
>     if (IsNegative)
>         Shift = (-AP2).logBase2() - (-AP1).logBase2();
>     else
>         Shift = AP2.logBase2() - AP1.logBase2();
>
>>
>> -  // Use lshr here, since we've canonicalized to +ve numbers.
>> -  if (AP1 == AP2.lshr(Shift))
>> +  // Revert the canonicalization if both AP1 and AP2 are negative,
>> +  // as (-AP2 >> Shift == -AP1) doesn't imply (AP2 >> Shift == AP1).
>> +  bool CanFoldCompare = false;
>> +  if (AreBothNegative) {
>> +    AP1 = -AP1;
>> +    AP2 = -AP2;
>> +    CanFoldCompare = AP1 == AP2.ashr(Shift);
>> +  } else
>> +    CanFoldCompare = AP1 == AP2.lshr(Shift);
>> +
>> +  if (CanFoldCompare)
>>      return getICmp(I.ICMP_EQ, A, ConstantInt::get(A->getType(), Shift));
>
> And now this is more clear:
>
>     if (IsAShr ? AP1 == AP2.ashr(Shift) : AP1 == AP2.lshr(Shift))
>       return getICmp(I.ICMP_EQ, A, ConstantInt::get(A->getType(), Shift));
>
>>
>>    // Shifting const2 will never be equal to const1.
>> Index: test/Transforms/InstCombine/icmp-shr.ll
>> ===================================================================
>> --- test/Transforms/InstCombine/icmp-shr.ll
>> +++ test/Transforms/InstCombine/icmp-shr.ll
>> @@ -675,3 +675,19 @@
>>   %cmp = icmp ne i8 %shr, -30
>>   ret i1 %cmp
>>  }
>> +
>> +; Don't try to fold the entire body of function @PR20945 into a
>> +; single `ret i1 true` statement.
>> +; If %B is equal to 1, then this function would return false.
>> +; As a consequence, the instruction combiner is not allowed to fold %cmp
>> +; to 'true'. Instead, it should replace %cmp with a simpler comparison
>> +; between %B and 1.
>> +
>> +; CHECK-LABEL: @PR20945(
>> +; CHECK: icmp ne i32 %B, 1
>> +define i1 @PR20945(i32 %B) {
>> +  %shr = ashr i32 -9, %B
>> +  %cmp = icmp ne i32 %shr, -5
>> +  ret i1 %cmp
>> +}
>> +
>
> Also, please remove the blank line at the end of the file.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits