[PATCH] D14496: X86: More efficient codegen for 64-bit compares on 32-bit target

[David Kreitzer via llvm-commits]

DavidKreitzer added inline comments.

================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:375
@@ -374,1 +374,3 @@
 
+    /// Like SetCC, but wit LHS and RHS split into a high and low part. Ops #0
+    /// and #1 are the high and low bits of LHS, respectively. Ops #2 and #3
----------------
wit --> with

================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:378
@@ +377,3 @@
+    /// are the high and low bits of RHS. Op #4 is the condition code.
+    SETCC_PARTS,
+
----------------
I think the operand ordering ought to be [LHSLo, LHSHi, RHSLo, RHSHi] to be consistent with SHL_PARTS, et al.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2685
@@ -2684,2 +2684,3 @@
 
+
   // FIXME: This generated code sucks.
----------------
Should probably delete the extra blank line.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2749
@@ +2748,3 @@
+
+  if (TLI.getOperationAction(ISD::SETCC_PARTS, LHSLo.getValueType()) ==
+      TargetLowering::Custom) {
----------------
This looks reasonable to me.

I am just wondering whether it would be better to refactor this such that we always generate SETCC_PARTS here regardless of whether the target has a custom lowering and then implement a default lowering that does most of what this routine is currently doing.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:14584
@@ +14583,3 @@
+  // other relations, we need to flip the operands.
+  SDValue LHSHi = Op.getOperand(0);
+  SDValue RHSHi = Op.getOperand(1);
----------------
Your operand ordering for SETCC_PARTS doesn't match your comment in ISDOpcodes.h. The two should at least be in sync, and I already gave my opinion on what the operand order ought to be. :-)

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:14614
@@ +14613,3 @@
+
+  SDVTList VTs = DAG.getVTList(LHSHi.getValueType(), MVT::i32);
+  SDValue LowSub = DAG.getNode(X86ISD::SUB, DL, VTs, LHSLo, RHSLo);
----------------
The code sequences you are generating here look great!

================
Comment at: test/CodeGen/X86/wide-integer-cmp.ll:38
@@ +37,3 @@
+; CHECK: movl 4(%esp), [[LHSLo:%[a-z]+]]
+; CHECK: movl 8(%esp), [[RHSHi:%[a-z]+]]
+; CHECK: cmpl 12(%esp), [[LHSLo]]
----------------
I think you meant LHSHi here.

================
Comment at: test/CodeGen/X86/wide-integer-cmp.ll:60
@@ +59,3 @@
+; CHECK: movl 12(%esp), [[RHSLo:%[a-z]+]]
+; CHECK: movl 16(%esp), [[LHSHi:%[a-z]+]]
+; CHECK: cmpl 4(%esp), [[RHSLo]]
----------------
Again, this looks odd. Did you mean RHSHi here and at line 62?


================
Comment at: test/CodeGen/X86/wide-integer-cmp.ll:79
@@ +78,3 @@
+; CHECK: movl 12(%esp), [[RHSLo:%[a-z]+]]
+; CHECK: movl 16(%esp), [[LHSHi:%[a-z]+]]
+; CHECK: cmpl 4(%esp), [[RHSLo]]
----------------
Also RHSHi here and at line 81.



http://reviews.llvm.org/D14496