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

src/codegen-ia32.cc

 // Copyright 2006-2008 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 1167 matching lines @@
            strict_ ? "true" : "false");
   }
 #endif
 };
 
 
 void CodeGenerator::Comparison(Condition cc, bool strict) {
   // 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_not_smi(this);
+  JumpTarget done(this);
+  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();
 
-  // Check for the smi case.
-  JumpTarget is_smi(this);
-  JumpTarget done(this);
-  __ mov(ecx, Operand(eax));
-  __ or_(ecx, Operand(edx));
-  __ test(ecx, Immediate(kSmiTagMask));
-  is_smi.Branch(zero, taken);
-
+  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;
 }
 
 
 class DeferredSmiComparison: public DeferredCode {
  public:
   DeferredSmiComparison(CodeGenerator* generator,
                         Condition cc,
                         bool strict,
@@ skipping 2934 matching lines @@
     case Token::GT:
       cc = greater;
       break;
     case Token::LTE:
       cc = less_equal;
       break;
     case Token::GTE:
       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.

       cc_reg_ = zero;
       return;
     }
     default:
       UNREACHABLE();
   }
 
   // Optimize for the case where (at least) one of the expressions
   // is a literal small integer.
   if (IsInlineSmi(left->AsLiteral())) {
     Load(right);
     SmiComparison(ReverseCondition(cc), left->AsLiteral()->handle(), strict);
     return;
   }
   if (IsInlineSmi(right->AsLiteral())) {
     Load(left);
     SmiComparison(cc, right->AsLiteral()->handle(), strict);
     return;
   }
 
-  VirtualFrame::SpilledScope spilled_scope(this);
-  LoadAndSpill(left);
-  LoadAndSpill(right);
+  Load(left);
+  Load(right);
   Comparison(cc, strict);
 }
 
 
 bool CodeGenerator::IsActualFunctionReturn(JumpTarget* target) {
   return (target == &function_return_ && !function_return_is_shadowed_);
 }
 
 
 #ifdef DEBUG
@@ skipping 1542 matching lines @@
 
   // Slow-case: Go through the JavaScript implementation.
   __ bind(&slow);
   __ InvokeBuiltin(Builtins::INSTANCE_OF, JUMP_FUNCTION);
 }
 
 
 #undef __
 
 } }  // namespace v8::internal