[PATCH] D159464: [InstCombine] Fold comparison of adding two zero extended booleans

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2905
+  // We have two cases to handle here:
+  // 1- (zext i1 X + zext i1 Y) == 1 --> xor i1 X, Y
+  // 2- (zext i1 X + zext i1 Y) == 0 --> !(or i1 X, Y)
----------------
Add `(zext i1 X + zext i1 Y) == 2 --> and i1 X, Y`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2906
+  // 1- (zext i1 X + zext i1 Y) == 1 --> xor i1 X, Y
+  // 2- (zext i1 X + zext i1 Y) == 0 --> !(or i1 X, Y)
+  if (Cmp.isEquality()) {
----------------
Can also do:

`(sext i1 X + sext i1 Y) == 0 --> !(or i1 X, Y)`
`(sext i1 X + sext i1 Y) == 1 --> false`
`(sext i1 X + sext i1 Y) == 2 --> false`

`(sext i1 X + zext i1 Y) == 0 --> (xor i1 X, Y)`
`(sext i1 X + zext i1 Y) == 1 --> Y`
`(sext i1 X + zext i1 Y) == 2 --> false`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2909
+    if (match(Add, m_c_Add(m_OneUse(m_ZExt(m_Value(X))),
+                           m_OneUse(m_ZExt(m_Value(Y))))) &&
+        X->getType()->isIntOrIntVectorTy(1) &&
----------------
Somewhat nit, but I'm not a fan of using `X` and `Y` here. While it fallsthrough to nullptr now, somewhat may change that not realizing its necessary for correctness as `X` and `Y` may be overwritten.

Can you change to maybe `Xi1` and `Yi1` or really any temporary name you want.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2919
+        Cond = Builder.CreateNot(Builder.CreateOr(X, Y));
+      }
+      return replaceInstUsesWith(Cmp, Cond);
----------------
nit: No need for braces. Personally would do:
```
Case 1:
if(C.isOne())
  Cond = Builder.CreateXor(...)
Case 2:
else
   Cond = Builder.CreateNot(...)
...
```


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

https://reviews.llvm.org/D159464