[PATCH] D12136: [X86] Emit more efficient >= comparisons against 0

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:24077
@@ -24076,1 +24076,3 @@
 
+static SDValue foldXorTruncShiftIntoCmp(SDNode *N, SelectionDAG &DAG) {
+  // This is only worth doing if the output type is i8.
----------------
mbodart wrote:
> It would be nice to have a brief comment here describing the general pattern.
> Something like:
> 
> Attempt to transform signedness tests similar to (i8)(X >= 0), appearing in DAG form
> as for example  (xor i8  trunc(A i32 >> 31), 1), into setcc A SETGT -1.
Sure, will do!

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:24078
@@ +24077,3 @@
+static SDValue foldXorTruncShiftIntoCmp(SDNode *N, SelectionDAG &DAG) {
+  // This is only worth doing if the output type is i8.
+  if (N->getValueType(0) != MVT::i8)
----------------
mbodart wrote:
> I would think we also want to suppress this if the xor has more than one user.
It might be a little early for me (no coffee, etc.), why is it undesirable to use a `test`/`setns` sequence if there is more than one user?

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:24113
@@ +24112,3 @@
+  SDValue Cond = DAG.getSetCC(DL, MVT::i8, ShiftOp,
+                              DAG.getConstant(-1, DL, ShiftOpTy), ISD::SETGT);
+  return Cond;
----------------
mbodart wrote:
> This is fine.  But just curious, is there a reason you chose SETGT -1 over SETGE 0?
We seem to normalize it to that: http://llvm.org/docs/doxygen/html/X86ISelLowering_8cpp_source.html#l03732

Using SETGE 0 works equally well but results in a `test`/`setge` sequence.  The sequence is equivalent but also requires remembering that `test` sets OF to 0.


http://reviews.llvm.org/D12136