[PATCH] D133919: [InstCombine] Fold ((x?1:4)&(y?1:4))==0 to x^y

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:6480
+    // (icmp eq (and (select A, C, D), (select B, C, D)), 0) --> (xor A, B)
+    if (I.getPredicate() == ICmpInst::ICMP_EQ && match(Op0,
+      m_c_And(
----------------
The formatting is off.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:6481-6483
+      m_c_And(
+        m_Select(m_Value(A), m_Value(C), m_Value(D)),
+        m_Select(m_Value(B), m_Deferred(C), m_Deferred(D))
----------------
This shouldn't use the commutative matcher (m_c_And); it should be m_And() with m_Specific(). 
Ie, we match operand 0 of the 'and' as any select; then, we match operand 1 with constraints based on whatever was matched as operand 0.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:6485
+      )
+    ) && match(Op1, m_Zero()) && isKnownNonZero(C, DL, /*Depth*/0, &AC, &I, &DT) && isKnownNonZero(D, DL, /*Depth*/0, &AC, &I, &DT) && haveNoCommonBitsSet(C, D, DL, &AC, &I, &DT)) {
+      return BinaryOperator::CreateXor(A, B);
----------------
If we're using the more general ValueTracking API's, then there should be at least one test for each code path that uses non-constants to exercise that more expensive analysis.

This patch is getting bigger than I originally anticipated, so I'd be fine with making that a small follow-up patch, but see if @RKSimon agrees.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:6488
+    }
+    // (icmp ne (and (select A, C, D), (select B, C, D)), 0) --> (xot (xor A, B), true)
+    if (I.getPredicate() == ICmpInst::ICMP_NE && match(Op0,
----------------
xot -> not ?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:6490-6492
+      m_OneUse(m_c_And(
+        m_OneUse(m_Select(m_Value(A), m_Value(C), m_Value(D))),
+        m_OneUse(m_Select(m_Value(B), m_Deferred(C), m_Deferred(D)))
----------------
It shouldn't matter whether the selects have other uses. We're creating 2 new instructions, so if the 'and' has only one use, then and+icmp will be deleted, and the transform doesn't increase instruction count.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133919