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

[Maksim Kita via Phabricator via llvm-commits]

kitaisreal added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2041
+  SmallVector<std::pair<Value *, Value *>, 2> CmpValues;
+  SmallVector<llvm::Value *, 16> WorkList(1, Or);
+
----------------
nikic wrote:
> Unnecessary `llvm::` prefix here and elsewhere.
Updated.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2052
+    Value *OrLhs = OrOperator->getOperand(0),
+          *OrRhs = OrOperator->getOperand(1);
+
----------------
nikic wrote:
> goldstein.w.n wrote:
> > Prefer not name these `Or*` as you expect to match them for `Xor`. Can you just make these `Lhs`/`Rhs`.
> These are the Lhs/Rhs of an Or though. Lhs/Rhs of the Xor are matched below.
> 
> Not sure why this isn't doing `match(CurrentValue, m_Or(m_Value(OrLhs), m_Value(OrRhs))` though.
Updated implementation to use `match`.


================
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);
----------------
goldstein.w.n wrote:
> The `match(Op, Xor) { emplace(XorOps) } else { WorkList.Add(Op) }` could be a lambda for LHS/RHS.
Updated.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2068
+
+  if (Match) {
     auto BOpc = Pred == CmpInst::ICMP_EQ ? Instruction::And : Instruction::Or;
----------------
goldstein.w.n wrote:
> 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.
It seems that more easier approach is to clear `CmpValues` if we fail to match.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2080
+                                       CmpValues.begin()->second);
+    return BinaryOperator::Create(BOpc, LhsCmp, RhsCmp);
   }
----------------
nikic wrote:
> Why does this one need to be handled separately?
To properly support lifetime of allocated operators we must allocate them using `Builder`, and `Builder` methods return `Value *`.
But this function expects to return `BinaryOperator *` instead of `Value *`, so last one handled separately.


================
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;
----------------
goldstein.w.n wrote:
> 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.
Added TODO.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-or.ll:415
+  ret i1 %cmp
+}
----------------
goldstein.w.n wrote:
> 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.
I moved tests into separate patch https://reviews.llvm.org/D154384. Should we merge it first so we can see the difference after this patch is applied ?


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

https://reviews.llvm.org/D154306