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

[Duncan P. N. Exon Smith]

> On 2014-Sep-16, at 03:25, Andrea Di Biagio <Andrea_DiBiagio at sn.scee.net> wrote:
> 
> Updated patch that addresses the comments from Suyog.
> 
> http://reviews.llvm.org/D5356
> 
> Files:
>  lib/Transforms/InstCombine/InstCombineCompares.cpp
>  test/Transforms/InstCombine/icmp-shr.ll
> <D5356.13741.patch>

(Sorry for the delay, just got back from vacation.)

Thanks for finding and fixing this!

This basically LGTM, but it's confusing to switch the signs to calculate
the log and then switch back.  It's hard to know what `AP1` and `AP2`
refer to.

Given that they cannot really be canonicalized to +ve numbers, I prefer
leaving them untouched.  Details below.

> Index: lib/Transforms/InstCombine/InstCombineCompares.cpp
> ===================================================================
> --- lib/Transforms/InstCombine/InstCombineCompares.cpp
> +++ lib/Transforms/InstCombine/InstCombineCompares.cpp
> @@ -1085,6 +1085,7 @@
>      return getICmp(I.ICMP_EQ, A, ConstantInt::getNullValue(A->getType()));
>    }
>  
> +  bool AreBothNegative = false;

It's simpler to name this `IsNegative`, since if only one is negative
we'll hit an early return and don't need this variable.

    bool IsNegative = false;

(Up to you though.)

>    if (IsAShr) {
>      if (AP1.isNegative() != AP2.isNegative()) {
>        // Arithmetic shift will never change the sign.
> @@ -1095,6 +1096,7 @@
>      if (AP1.isNegative()) {
>        AP1 = -AP1;
>        AP2 = -AP2;
> +      AreBothNegative = true;
>      }

This is the key thing I would do differently.  Stop changing `AP1` and
`AP2`, making this simply:

    if (AP1.isNegative())
      IsNegative = true;

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);
    }

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