[PATCH] D144777: [InstCombine] Fold signbit test of a pow2 or zero

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

We should have 2 more tests: (1) extra use of the `sub` and (2) extra use of the `and`.
As noted earlier, this patch should not require a "m_OneUse" limitation. I realize that it looks like a regression for the existing test, but that should be ok when viewed globally: we are reducing to an equality compare, and GVN, CVP, or some other pass will reduce that to the optimal form. It's just lucky (or unlucky) that the min/max folds added with D144606 <https://reviews.llvm.org/D144606> are able to reduce it all within InstCombine.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1853
+  {
+    Value* X;
+    if (match(And, m_OneUse(m_c_And(m_Neg(m_Value(X)), m_Deferred(X))))) {
----------------
Formatting:
  Value *X;


================
Comment at: llvm/test/Transforms/InstCombine/fold-signbit-test-power2.ll:5
 ; icmp slt (and X, -X), 0 --> icmp eq (X, MinSignC)
 define i1 @pow2_or_zero1(i8 %x) {
 ; CHECK-LABEL: @pow2_or_zero1(
----------------
It's easier to follow the test progression if we make the test names slightly more specific. 
This is a signbit test for "isNegative", so "pow2_or_zero_is_negative".
To provide a little more coverage, you could change some of the tests to include variations of that like:

```
define i1 @pow2_or_zero_is_negative(i8 %x) {
  %negx = sub i8 0, %x
  %pow2_or_zero = and i8 %negx, %x 
  %cmp = icmp ugt i8 %pow2_or_zero, 127
  ret i1 %cmp
}
```

That should already fold as expected without having to change anything in this patch, but it's good to show that we can handle it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144777