[PATCH] D154306: [InstCombine] Generalise ((x1 ^ y1) | (x2 ^ y2)) == 0 transform to more than two pairs of variables

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2052
+    Value *OrLhs = OrOperator->getOperand(0),
+          *OrRhs = OrOperator->getOperand(1);
+
----------------
Prefer not name these `Or*` as you expect to match them for `Xor`. Can you just make these `Lhs`/`Rhs`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2061
+
+    if (match(OrRhs, m_OneUse(m_Xor(m_Value(Lhs), m_Value(Rhs))))) {
+      CmpValues.emplace_back(Lhs, Rhs);
----------------
The `match(Op, Xor) { emplace(XorOps) } else { WorkList.Add(Op) }` could be a lambda for LHS/RHS.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2068
+
+  if (Match) {
     auto BOpc = Pred == CmpInst::ICMP_EQ ? Instruction::And : Instruction::Or;
----------------
At first I thought this should be `!CmpValues.empty()` so you can match something like:

`(or (or (xor A, B), (xor C, D)), (and E, F))` but realize we will visit the inner or and handle it there. Can you add a comment here (or above where you set `Match` to false) that such a case will inherently be handled so its okay to fail on any non-or/xor.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2038
+  // ((X1 ^ X2) || (X3 ^ X4) || (X5 ^ X6)) == 0 --> (X1 == X2) && (X3 == X4) && (X5 == X6)
+  // ((X1 ^ X2) || (X3 ^ X4) || (X5 ^ X6)) != 0 --> (X1 != X2) || (X3 != X4) || (X5 != X6)
+  bool Match = false;
----------------
kitaisreal wrote:
> goldstein.w.n wrote:
> > Maybe add as a todo, but this also works for `sub` (or implement in a follow up patch!)
> I can implement this in follow up patch.
Thanks :)
Can you add `TODO` for it in the meantime.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-or.ll:415
+  ret i1 %cmp
+}
----------------
goldstein.w.n wrote:
> Can you 1) split the tests to a seperate patch (so we can see the diff this patch generates).
> 2) Can you add a few negative tests (maybe where cmp is not against 0, one of the xors has multiuse, where its or->or (invalid), etc...).
This is still outstanding.


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

https://reviews.llvm.org/D154306