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

[Andrea Di Biagio]

Hi Suyog,

Thanks for the useful comments/suggestions!
I will upload another patch soon.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1123
@@ +1122,3 @@
+  // but
+  //   9 >> 1 != 5.
+  bool CanFoldCompare = false;
----------------
suyog wrote:
> 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)
Ah yes. I was not very happy about my original comment here... I like your suggestion. I will fix.

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

================
Comment at: test/Transforms/InstCombine/icmp-shr.ll:704
@@ +703,2 @@
+}
+
----------------
suyog wrote:
> 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
>   }
True. I will upload a new patch with the simplified test case (I will also address the other comments).

http://reviews.llvm.org/D5356