[PATCH] D19087: [x86, ppc] prefer comparisons against zero for and+cmp sequences

[Sanjay Patel via llvm-commits]

spatel added a comment.

Before abandoning ship, provide inline answers to Kevin's questions.


================
Comment at: include/llvm/Target/TargetLowering.h:308
@@ -307,1 +307,3 @@
 
+  /// Return true if the target should transform:
+  /// X & Y == Y
----------------
kbsmith1 wrote:
> I see \brief used in other comments preceding functions.  Should that be here as well?
Doxygen's autobrief was enabled some time in the last few months, so \brief is just noise as I understand it. We can remove \brief from documentation comments for readability.

================
Comment at: include/llvm/Target/TargetLowering.h:313
@@ +312,3 @@
+  ///
+  /// If all bits of X that are masked by Y are set, then all bits in the
+  /// bitwise-not of X that are masked by Y are not set.
----------------
kbsmith1 wrote:
> This specific statement is a little hard to follow.  I think it might be clearer if you say X & Y == Y implies that the bits in Y are a subset of the bits in X.  Therefore, the set of bits not in X (~X) unioned (&) with
> the bits in Y must be the empty set (0).  So, X & Y == 0 is equivalent to ~X & Y == 0.
This may be a case where we should just let the equation speak for itself. :)

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5174
@@ +5173,3 @@
+  ICmpInst::Predicate P;
+  if (!match(ICmp, m_ICmp(P, m_And(m_Value(X), m_Specific(Op1)), m_Value(Y))) &&
+      !match(ICmp, m_ICmp(P, m_And(m_Specific(Op1), m_Value(X)), m_Value(Y))) &&
----------------
kbsmith1 wrote:
> Maybe I am missing something, or don't understand, but I don't see anything in this routine that checks to make sure this is an == comparison.
The equality check is (poorly) placed on the first line.

================
Comment at: test/CodeGen/X86/bmi.ll:174
@@ -175,3 +173,3 @@
   %and = and i32 %y, %x
   %cmp = icmp ne i32 %and, %y
   ret i1 %cmp
----------------
kbsmith1 wrote:
> Is this valid for ne?  The comment in TargetLowering.h should discuss that.  Seems like it is, but it would be worth showing that in the comment.  This test should also have some tests that match the pattern, except use le, ge, lt, gt as well, as those are not legal patterns to hit the optimization.
Yes, the transform is valid for 'ne'. This patch is just undoing the opposite direction transform performed by InstCombine in visitICmpInstWithInstAndIntCst(). But I agree we should have tests to confirm that the transform doesn't fire for lt/gt.


http://reviews.llvm.org/D19087