[PATCH] D62818: [InstCombine] Introduce fold for icmp pred (and X, (sh signbit, Y)), 0.

[Huihui Zhang via Phabricator via llvm-commits]

huihuiz added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1796
+                                       m_LShr(m_Value(), m_Value()))),
+                       m_OneUse(m_Value(V0)))),
+                   m_CombineAnd(m_Zero(), m_Value(Zero)))) &&
----------------
lebedev.ri wrote:
> I'm not sure why i have added `m_OneUse()` here, it should not be here.
I agree that m_OneUse() should not be in the pattern matching. V0 might be constant value, which will have more than one use outside of its current function.

Actually I added, not you, sorry about that.

There is a regression, see test file: test/Transforms/InstCombine/onehot_merge.ll

```
define i1 @foo1_and(i32 %k, i32 %c1, i32 %c2) {
bb:
  %tmp = shl i32 1, %c1
  %tmp4 = lshr i32 -2147483648, %c2
  %tmp1 = and i32 %tmp, %k
  %tmp2 = icmp eq i32 %tmp1, 0
  %tmp5 = and i32 %tmp4, %k
  %tmp6 = icmp eq i32 %tmp5, 0
  %or = or i1 %tmp2, %tmp6
  ret i1 %or
}
```

failed to fold 
(iszero(A&K1) | iszero(A&K2)) ->  (A&(K1|K2)) != (K1|K2) , where K1 and K2 are 'one-hot' (only one bit is on). 
Here K1 is one, K2 is signbit.

I am still thinking how to get over this regression.


================
Comment at: test/Transforms/InstCombine/signbit-shl-and-icmpeq-zero.ll:180
 
 ; Negative tests
 
----------------
spatel wrote:
> I didn't step through the transforms, but it seems wrong to call this a 'negative test'. This patch must have fired and allowed further simplification?
Yes, X being constant is positive case. Fold happened, and allowed further simplification.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62818