[PATCH] D62818: [InstCombine] Fix fold order for icmp pred (and (sh X, Y), C), 0.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Nice, getting closer.
Could you please split this up:

1. A patch that adds your original motivational testcase that shows that the fold order is wrong.
2. The move of the `// Replace (and X, (1 << size(X)-1) != 0) with x s< 0` fold
3. A patch with just new `test/Transforms/InstCombine/icmp-shift-and-signbit.ll`
4. The fold itself, showing the changes to the check lines



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1646
 
+  // If X is shift instruction, codegen is better when fold
+  // (V0 << V1) & signbit != 0 into (V0 << V1) s< 0
----------------
Here the codegen is irrelevant.
We do this because it results in simpler IR.
Not sure if that new comment adds anything useful


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1660
+    // ((X & ~7) == 0) --> X < 8
+    if ((~(*C2) + 1).isPowerOf2()) {
+      Constant *NegBOC =
----------------
`C2->negate().isPowerOf2()`


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2791-2792
-
-      // Replace (and X, (1 << size(X)-1) != 0) with x s< 0
-      if (BOC->isSignMask()) {
-        Constant *Zero = Constant::getNullValue(BOp0->getType());
----------------
Uhm, where did this check that we were comparing with `0`?


================
Comment at: test/Transforms/InstCombine/icmp-shift-and-signbit.ll:2
+; RUN: opt %s -instcombine -S | FileCheck %s
+
+
----------------
Please
1. Move this to a new differential
2. In that same patch, re-add your initial motivational pattern, that shows that fold reordering did something
3. Use `llvm/utils/update_test_checks.py` to generate check lines
4. Rebase *this* diff ontop of that new patch, so this diff shows how the check lines change



================
Comment at: test/Transforms/InstCombine/icmp-shift-and-signbit.ll:13
+  %tobool = icmp ne i32 %and, 0
+  %selv = select i1 %tobool, i32 %a, i32 %b
+  ret i32 %selv
----------------
`select` is not relevant for this pattern, drop it


================
Comment at: test/Transforms/InstCombine/icmp-shift-and-signbit.ll:68
+}
+
+
----------------
You also want a few extra tests:
1. A trivial vector test with `<i32 -2147483648, i32 -2147483648>` and `<i32 0, i32 0>`
2. 3 vector tests with undefs:
   * `<i32 -2147483648, i32 undef, i32 -2147483648>` and `<i32 0, i32 0, i32 0>`
   * `<i32 -2147483648, i32 -2147483648, i32 -2147483648>` and `<i32 0, i32 undef, i32 0>`
   * `<i32 -2147483648, i32 undef, i32 -2147483648>` and `<i32 0, i32 undef, i32 0>`
3. A tests to verify single-use constraints:
   * a test with extra use on `%shr` (should get folded, but not others)
   * a test with extra use on `%and`
   * a test with extra use on `%shr` and `%and`.

   How to introduce extra uses see e.g. `llvm/test/Transforms/InstCombine/unfold-masked-merge-with-const-mask-scalar.ll`



================
Comment at: test/Transforms/InstCombine/pr17827.ll:66
 
 ; Unsigned compare allows a transformation to compare against 0.
 define i1 @test_shift_and_cmp_changed2(i8 %p) {
----------------
These don't look like improvements to me.
Looks like that reordering exposes yet another missing fold.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62818/new/

https://reviews.llvm.org/D62818