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

[suyog]

Hi Andrea,

Thanks for pointing out the bug. I went through the code and the example and 
i agree that we need to revert the canonicalization if both AP1 and AP2 are negative.
Your patch seems ok to me, though i would like others to approve it. 

Also i had few suggestions for comments and test case in the patch which i noted inline 
(feel free to edit if they seem good to you).

I would like Duncan to review this for final commit as we had worked on the original patch.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1123
@@ +1122,3 @@
+  // but
+  //   9 >> 1 != 5.
+  bool CanFoldCompare = false;
----------------
This comment can be made crisp. Something like - Revert the canonicalization if both AP1 and AP2 are negative,
as (-AP2 >> Shift == -AP1) is not equal to (AP2 >> Shift == AP1)

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1126
@@ +1125,3 @@
+  if (AreBothNegative) {
+    // Undo the canonicalization.
+    AP1 = -AP1;
----------------
This comment can be removed.

================
Comment at: test/Transforms/InstCombine/icmp-shr.ll:704
@@ +703,2 @@
+}
+
----------------
This test case can be simplified (exclude if-else part as patch focuses on icmp result)

  ; CHECK-LABEL: @negative_constants(
  ; CHECK: icmp eq i32 %B, 1
  define i1 @negative_constants(i32 %B) {
    %shr = ashr i32 -9, %B
    %cmp = icmp ne i32 %shr, -5
    ret i1 %cmp
  }

http://reviews.llvm.org/D5356