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

[Kevin B. Smith via llvm-commits]

kbsmith1 added a comment.

Replied to inline comments.


================
Comment at: include/llvm/Target/TargetLowering.h:308
@@ -307,1 +307,3 @@
 
+  /// Return true if the target should transform:
+  /// X & Y == Y
----------------
spatel wrote:
> 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.
I didn't realize that.  Not too long ago a reviewer had me add \brief in some code I was doing.

================
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.
----------------
spatel wrote:
> 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. :)
That sounds fine.

================
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))) &&
----------------
spatel wrote:
> 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.
I see it now.  I skipped right by it.


http://reviews.llvm.org/D19087