[PATCH] D49954: [InstCombine] Fold Select with Xor condition

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:68
+  CmpInst::Predicate Pred;
+  if (!match(Cond, m_c_ICmp(Pred, m_Value(X), m_Zero())) ||
+      Pred != ICmpInst::ICMP_EQ)
----------------
Constants are always canonicalized to RHS, so no commutativity in here.
https://rise4fun.com/Alive/r7Q


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:1736
+  if (Value *V =
+          foldSelectInstWithBinaryOp(CondVal, TrueVal, FalseVal, Builder))
+    return replaceInstUsesWith(SI, V);
----------------
I'm not sure there is much point in not passing the `SelectInst &SI` itself.
It's certainly a bit harder to read `foldSelectInstWithBinaryOp()` itself if it does lot's of matching on a number of arguments
(as opposed to one huge `match()` on one single function argument)


================
Comment at: test/Transforms/InstCombine/select-xor-icmp.ll:24
+;
+  %A = icmp eq <2 x i8>  %x, <i8 0, i8 0>
+  %B = xor <2 x i8>  %x, %z
----------------
Since there is a constant, please also add a test with `<i8 0, i8 undef, i8 0>`


================
Comment at: test/Transforms/InstCombine/select-xor-icmp.ll:36
+;
+  %A = icmp eq i32 0, %x
+  %B = xor i32 %x, %z
----------------
This never happens


================
Comment at: test/Transforms/InstCombine/select-xor-icmp.ll:62
+;
+  %A = icmp ne i32 %x, 0
+  %B = xor i32 %x, %z
----------------
Please also add one test with non-zero


Repository:
  rL LLVM

https://reviews.llvm.org/D49954