[PATCH] D144608: [InstCombine] Add transforms for `(icmp {u|s}ge/le (xor X, Y), X)`

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:4091-4093
+  if (!match(Op0, m_c_Xor(m_Specific(Op1), m_Value(A))) &&
+      !match(Op1, m_c_Xor(m_Specific(Op0), m_Value(A))))
+    return nullptr;
----------------
Can we canonicalize the xor as operand 0?
It shouldn't make much difference on this patch, but that would make the next one smaller.

  // Normalize xor operand as operand 0.
  CmpInst::Predicate Pred = I.getPredicate();
  if (match(Op1, m_c_Xor(m_Specific(Op0), m_Value()))) {
    std::swap(Op0, Op1);
    Pred = ICmpInst::getSwappedPredicate(Pred);
  }
  if (!match(Op0, m_c_Xor(m_Specific(Op1), m_Value(A))))
    return nullptr;



================
Comment at: llvm/test/Transforms/InstCombine/icmp-of-xor-x.ll:41
   %y = or <2 x i8> %yy, <i8 9, i8 8>
   %xor = xor <2 x i8> %x, %y
   %r = icmp ule <2 x i8> %xor, %x
----------------
This is testing the commuted xor, so that's good  - but it would be better to commute the xor operands in the original test, so we are not relying on some other transform to get to canonical form.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-of-xor-x.ll:72
   %xor = xor i8 %x, %y
   %r = icmp sge i8 %xor, %x
   ret i1 %r
----------------
Similar to above, it would be better if the xor operands are swapped originally.
But we need another instruction to make this test provide coverage for the pattern where the xor is operand 1 of the icmp. Ie, %x needs to be generated by a binop similar to the test just above this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144608