Use unspilled virtual frames when compiling compare operations....

src/codegen-ia32.cc

Index: src/codegen-ia32.cc
===================================================================
--- src/codegen-ia32.cc	(revision 1019)
+++ src/codegen-ia32.cc	(working copy)
@@ -1185,40 +1185,62 @@
   // Strict only makes sense for equality comparisons.
   ASSERT(!strict || cc == equal);
 
+  Result left_side(this);
+  Result right_side(this);
   // Implement '>' and '<=' by reversal to obtain ECMA-262 conversion order.
   if (cc == greater || cc == less_equal) {
     cc = ReverseCondition(cc);
-    frame_->EmitPop(edx);
-    frame_->EmitPop(eax);
+    right_side = frame_->Pop();

[Kevin Millikin (Chromium), 2008/12/29 09:05:38]

I'm confused by this: the rhs is normally on top of the lhs on the stack, so I
would expect the rhs to be popped first in the normal case and the lhs to be
popped first in the reversed case.

[William Hesse, 2008/12/29 11:47:42]

True.  Left and right were consistently flipped through the entire function. 
Left has been renamed to right, and right to left, so now they are correct.

+    left_side = frame_->Pop();
   } else {
-    frame_->EmitPop(eax);
-    frame_->EmitPop(edx);
+    left_side = frame_->Pop();
+    right_side = frame_->Pop();
   }
-
+  right_side.ToRegister();
+  left_side.ToRegister();
+  ASSERT(right_side.is_valid());
+  ASSERT(left_side.is_valid());
   // Check for the smi case.
-  JumpTarget is_smi(this);
+  JumpTarget is_not_smi(this);
   JumpTarget done(this);
-  __ mov(ecx, Operand(eax));
-  __ or_(ecx, Operand(edx));
-  __ test(ecx, Immediate(kSmiTagMask));
-  is_smi.Branch(zero, taken);
+  Result temp = allocator_->Allocate();
+  ASSERT(temp.is_valid());
+  __ mov(temp.reg(), right_side.reg());
+  __ or_(temp.reg(), Operand(left_side.reg()));
+  __ test(temp.reg(), Immediate(kSmiTagMask));
+  temp.Unuse();
+  is_not_smi.Branch(not_zero, &left_side, &right_side, not_taken);
+  __ cmp(right_side.reg(), Operand(left_side.reg()));

[Kevin Millikin (Chromium), 2008/12/29 09:05:38]

Here is where the variable naming/popping order confusion manifests itself.  Why
is right_side on the left?

[William Hesse, 2008/12/29 11:47:42]

Done.

+  // Fall through to |done|.

[Kevin Millikin (Chromium), 2008/12/29 09:05:38]

Did we need to change the order we emit the smi and non-smi cases?  We've now
grown an extra unconditional jump on the fast path.  We should instead work hard
to make merging work properly with the original basic block order (and leave the
original order for now even if its temporarily suboptimal).

If you leave it as you've changed it, please update the comments (here we're not
falling through to done, we're jumping to it....).

[William Hesse, 2008/12/29 11:47:42]

Changed back, so there is not an unconditional jump on the fast case.
The JumpTarget "done" is bound to the slow case first, so we are probably
suboptimal until we handle merges better.

+  left_side.Unuse();
+  right_side.Unuse();
+  done.Jump();
 
+  is_not_smi.Bind(&left_side, &right_side);
   // When non-smi, call out to the compare stub.  "parameters" setup by
   // calling code in edx and eax and "result" is returned in the flags.
+

[Kevin Millikin (Chromium), 2008/12/29 09:05:38]

Remove the extra blank line.

[William Hesse, 2008/12/29 11:47:42]

Done.

+  if (!right_side.reg().is(eax)) {
+    left_side.ToRegister(eax);
+    right_side.ToRegister(edx);
+  } else if (!left_side.reg().is(edx)) {
+    right_side.ToRegister(edx);
+    left_side.ToRegister(eax);
+  } else {
+    frame_->Spill(eax);  // Can be multiply referenced, even now.
+    frame_->Spill(edx);
+    __ xchg(eax, edx);
+    // If right_side and left_side become real (non-dummy) arguments
+    // to CallStub, they need to be swapped in this case.
+  }
   CompareStub stub(cc, strict);
-  frame_->CallStub(&stub, 0);
+  Result answer = frame_->CallStub(&stub, &left_side, &right_side, 0);
   if (cc == equal) {
-    __ test(eax, Operand(eax));
+    __ test(answer.reg(), Operand(answer.reg()));
   } else {
-    __ cmp(eax, 0);
+    __ cmp(answer.reg(), 0);
   }
-  done.Jump();
-
-  // Test smi equality by pointer comparison.
-  is_smi.Bind();
-  __ cmp(edx, Operand(eax));
-  // Fall through to |done|.
-
+  answer.Unuse();
   done.Bind();
   cc_reg_ = cc;
 }
@@ -4173,17 +4195,15 @@
       cc = greater_equal;
       break;
     case Token::IN: {
-      VirtualFrame::SpilledScope spilled_scope(this);
-      LoadAndSpill(left);
-      LoadAndSpill(right);
+      Load(left);
+      Load(right);
       frame_->InvokeBuiltin(Builtins::IN, CALL_FUNCTION, 2);
-      frame_->EmitPush(eax);  // push the result
+      frame_->Push(eax);  // push the result

[Kevin Millikin (Chromium), 2008/12/29 09:05:38]

Get rid of this explicit (and non-counted) reference to eax in non-spilled code.
 Instead, VirtualFrame::InvokeBuiltin could return a result that can be used to
manage the result register resource (you should also get rid of the version of
VirtualFrame::Push that takes a register).

[William Hesse, 2008/12/29 11:47:42]

I would like to make this a separate change, to be done immediately.

       return;
     }
     case Token::INSTANCEOF: {
-      VirtualFrame::SpilledScope spilled_scope(this);
-      LoadAndSpill(left);
-      LoadAndSpill(right);
+      Load(left);
+      Load(right);
       InstanceofStub stub;
       frame_->CallStub(&stub, 2);
       __ test(eax, Operand(eax));

[Kevin Millikin (Chromium), 2008/12/29 09:05:38]

Ditto.

[William Hesse, 2008/12/29 11:47:42]

To be done in separate change.

@@ -4207,9 +4227,8 @@
     return;
   }
 
-  VirtualFrame::SpilledScope spilled_scope(this);
-  LoadAndSpill(left);
-  LoadAndSpill(right);
+  Load(left);
+  Load(right);
   Comparison(cc, strict);
 }