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

[Kevin B. Smith via llvm-commits]

kbsmith1 added a comment.

See inline comments.


================
Comment at: include/llvm/Target/TargetLowering.h:308
@@ -307,1 +307,3 @@
 
+  /// Return true if the target should transform:
+  /// X & Y == Y
----------------
I see \brief used in other comments preceding functions.  Should that be here as well?

================
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.
----------------
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.

================
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))) &&
----------------
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.

================
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
----------------
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.


http://reviews.llvm.org/D19087