Move SpilledFrame inside cases of SmiOperation....

src/codegen-ia32.cc

Index: src/codegen-ia32.cc
===================================================================
--- src/codegen-ia32.cc	(revision 1126)
+++ src/codegen-ia32.cc	(working copy)
@@ -189,12 +189,12 @@
       Result context = frame_->CallRuntime(Runtime::kNewContext, 1);
 
       if (kDebug) {
-        frame_->SpillAll();  // Needed for breakpoint below.
         JumpTarget verified_true(this);
         // Verify eax and esi are the same in debug mode
         __ cmp(context.reg(), Operand(esi));
         context.Unuse();
         verified_true.Branch(equal);
+        frame_->SpillAll();
         __ int3();
         verified_true.Bind();
       }
@@ -855,7 +855,7 @@
 
 class DeferredInlinedSmiAdd: public DeferredCode {
  public:
-  DeferredInlinedSmiAdd(CodeGenerator* generator, int value,
+  DeferredInlinedSmiAdd(CodeGenerator* generator, Smi* value,

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

While you're changing this code, you could clean it up to have one parameter per
line.

[William Hesse, 2012/05/07 09:35:10]

Done.

                         OverwriteMode overwrite_mode) :
       DeferredCode(generator), value_(value), overwrite_mode_(overwrite_mode) {
     set_comment("[ DeferredInlinedSmiAdd");
@@ -867,23 +867,23 @@
     arg.ToRegister();
     generator()->frame()->Spill(arg.reg());
     // Undo the optimistic add operation and call the shared stub.
-    __ sub(Operand(arg.reg()), Immediate(Smi::FromInt(value_)));
+    __ sub(Operand(arg.reg()), Immediate(value_));
     generator()->frame()->Push(&arg);
-    generator()->frame()->Push(Smi::FromInt(value_));
+    generator()->frame()->Push(value_);
     GenericBinaryOpStub igostub(Token::ADD, overwrite_mode_, SMI_CODE_INLINED);
     Result result = generator()->frame()->CallStub(&igostub, 2);
     exit()->Jump(&result);
   }
 
  private:
-  int value_;
+  Smi* value_;
   OverwriteMode overwrite_mode_;
 };
 
 
 class DeferredInlinedSmiAddReversed: public DeferredCode {
  public:
-  DeferredInlinedSmiAddReversed(CodeGenerator* generator, int value,
+  DeferredInlinedSmiAddReversed(CodeGenerator* generator, Smi* value,
                         OverwriteMode overwrite_mode) :
       DeferredCode(generator), value_(value), overwrite_mode_(overwrite_mode) {
     set_comment("[ DeferredInlinedSmiAddReversed");
@@ -893,11 +893,10 @@
     Result arg(generator());
     enter()->Bind(&arg);
     arg.ToRegister();
-    generator()->frame()->Spill(arg.reg());  // Should not be needed.
+    generator()->frame()->Spill(arg.reg());
     // Undo the optimistic add operation and call the shared stub.
-    Immediate immediate(Smi::FromInt(value_));
-    __ sub(Operand(arg.reg()), immediate);
-    generator()->frame()->Push(Smi::FromInt(value_));
+    __ sub(Operand(arg.reg()), Immediate(value_));
+    generator()->frame()->Push(value_);
     generator()->frame()->Push(&arg);
     GenericBinaryOpStub igostub(Token::ADD, overwrite_mode_, SMI_CODE_INLINED);
     arg = generator()->frame()->CallStub(&igostub, 2);
@@ -905,36 +904,37 @@
   }
 
  private:
-  int value_;
+  Smi* value_;
   OverwriteMode overwrite_mode_;
 };
 
 
 class DeferredInlinedSmiSub: public DeferredCode {
  public:
-  DeferredInlinedSmiSub(CodeGenerator* generator, int value,
+  DeferredInlinedSmiSub(CodeGenerator* generator,
+                        Smi* value,
                         OverwriteMode overwrite_mode) :
       DeferredCode(generator), value_(value), overwrite_mode_(overwrite_mode) {
     set_comment("[ DeferredInlinedSmiSub");
   }
 
   virtual void Generate() {
-    // The argument is actually passed in eax.
-    enter()->Bind();
-    VirtualFrame::SpilledScope spilled_scope(generator());
+    Result argument(generator());
+    enter()->Bind(&argument);
+    argument.ToRegister();
+    generator()->frame()->Spill(argument.reg());
     // Undo the optimistic sub operation and call the shared stub.
-    Immediate immediate(Smi::FromInt(value_));
-    __ add(Operand(eax), immediate);
-    generator()->frame()->EmitPush(eax);
-    generator()->frame()->EmitPush(immediate);
+    __ add(Operand(argument.reg()), Immediate(value_));
+    generator()->frame()->Push(&argument);
+    generator()->frame()->Push(value_);
     GenericBinaryOpStub igostub(Token::SUB, overwrite_mode_, SMI_CODE_INLINED);
-    generator()->frame()->CallStub(&igostub, 2);
+    Result answer = generator()->frame()->CallStub(&igostub, 2);
     // The result is actually returned in eax.

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

Remove this comment.

[William Hesse, 2012/05/07 09:35:10]

Done.

-    exit()->Jump();
+    exit()->Jump(&answer);
   }
 
  private:
-  int value_;
+  Smi* value_;
   OverwriteMode overwrite_mode_;
 };
 
@@ -943,29 +943,34 @@
  public:
   // tos_reg is used to save the TOS value before reversing the operands
   // eax will contain the immediate value after undoing the optimistic sub.
-  DeferredInlinedSmiSubReversed(CodeGenerator* generator, Register tos_reg,
+  DeferredInlinedSmiSubReversed(CodeGenerator* generator,
+                                Smi* value,
                                 OverwriteMode overwrite_mode) :
-      DeferredCode(generator), tos_reg_(tos_reg),
+      DeferredCode(generator), value_(value),
       overwrite_mode_(overwrite_mode) {
     set_comment("[ DeferredInlinedSmiSubReversed");
   }
 
   virtual void Generate() {
-    // The arguments are actually passed in eax and tos_reg.
-    enter()->Bind();
-    VirtualFrame::SpilledScope spilled_scope(generator());
+    Result argument(generator());
+    enter()->Bind(&argument);  // argument contains lhs - rhs.  lhs is value.

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

Try to make comments look like English sentences. You should try to capitalize
the first word.  "The argument contains the optimistic subtraction value - rhs."

+    argument.ToRegister();
+    generator()->frame()->Spill(argument.reg());

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

I'm confused by this spill.  Since argument is not written to, does it need to
be spilled? (Since it's not used as an "lvalue", does it even need to be a
register?)

     // Undo the optimistic sub operation and call the shared stub.
-    __ add(eax, Operand(tos_reg_));
-    generator()->frame()->EmitPush(eax);
-    generator()->frame()->EmitPush(tos_reg_);
+    Result temp = generator()->allocator()->Allocate();
+    ASSERT(temp.is_valid());
+    __ mov(temp.reg(), Immediate(value_));
+    __ sub(temp.reg(), Operand(argument.reg())); // lhs - (lhs - rhs) gives us rhs.

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

Line seems too long.  There should be two spaces between the semicolon and the
comment.

+    argument.Unuse();
+    generator()->frame()->Push(value_);
+    generator()->frame()->Push(&temp);
     GenericBinaryOpStub igostub(Token::SUB, overwrite_mode_, SMI_CODE_INLINED);
-    generator()->frame()->CallStub(&igostub, 2);
-    // The result is actually returned in eax.
-    exit()->Jump();
+    Result answer = generator()->frame()->CallStub(&igostub, 2);
+    exit()->Jump(&answer);
   }
 
  private:
-  Register tos_reg_;
+  Smi* value_;
   OverwriteMode overwrite_mode_;
 };
 
@@ -984,19 +989,19 @@
 
   // TODO(1217802): Optimize some special cases of operations
   // involving a smi literal (multiply by 2, shift by 0, etc.).
-  VirtualFrame::SpilledScope spilled_scope(this);
 
   // Get the literal value.
-  int int_value = Smi::cast(*value)->value();
+  Smi* smi_value = Smi::cast(*value);
+  int int_value = smi_value->value();
   ASSERT(is_intn(int_value, kMaxSmiInlinedBits));
 
   switch (op) {
     case Token::ADD: {
       DeferredCode* deferred = NULL;
       if (!reversed) {

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

Since this is a two-armed if anyway, it might read better if the condition were
not negated.

[William Hesse, 2012/05/07 09:35:10]

Done.

-        deferred = new DeferredInlinedSmiAdd(this, int_value, overwrite_mode);
+        deferred = new DeferredInlinedSmiAdd(this, smi_value, overwrite_mode);
       } else {
-        deferred = new DeferredInlinedSmiAddReversed(this, int_value,
+        deferred = new DeferredInlinedSmiAddReversed(this, smi_value,
                                                      overwrite_mode);
       }
       Result operand = frame_->Pop();
@@ -1013,31 +1018,42 @@
 
     case Token::SUB: {
       DeferredCode* deferred = NULL;
-      frame_->EmitPop(eax);
+      Result operand = frame_->Pop();
+      Result answer(this);  // Only allocated a new register if reversed.

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

allocated ==> allocate

[William Hesse, 2012/05/07 09:35:10]

Done.

+      operand.ToRegister();
+      frame_->Spill(operand.reg());

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

It looks like operand is only clobbered in the not-reversed case.  Could this
spill be done more lazily?

[William Hesse, 2012/05/07 09:35:10]

Done.

       if (!reversed) {
-        deferred = new DeferredInlinedSmiSub(this, int_value, overwrite_mode);
-        __ sub(Operand(eax), Immediate(value));
+        deferred = new DeferredInlinedSmiSub(this,
+                                             smi_value,
+                                             overwrite_mode);
+        __ sub(Operand(operand.reg()), Immediate(value));
+        answer = operand;
       } else {
-        deferred = new DeferredInlinedSmiSubReversed(this, edx, overwrite_mode);
-        __ mov(edx, Operand(eax));
-        __ mov(eax, Immediate(value));
-        __ sub(eax, Operand(edx));
+        answer = allocator()->Allocate();
+        ASSERT(answer.is_valid());
+        deferred = new DeferredInlinedSmiSubReversed(this,
+                                                     smi_value,
+                                                     overwrite_mode);
+        __ mov(answer.reg(), Immediate(value));
+        __ sub(answer.reg(), Operand(operand.reg()));
       }
-      deferred->enter()->Branch(overflow, not_taken);
-      __ test(eax, Immediate(kSmiTagMask));
-      deferred->enter()->Branch(not_zero, not_taken);
-      deferred->exit()->Bind();
-      frame_->EmitPush(eax);
+      operand.Unuse();
+      deferred->enter()->Branch(overflow, &answer, not_taken);

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

Maybe it makes this code uglier, but can't we pass operand instead of answer? 
In the non-reversed case, it's the same thing and in the deferred case it avoids
allocating a register, moving a constant into it, and subtracting, which seems
to be just recovering operand.

[William Hesse, 2012/05/07 09:35:10]

Done.

+      __ test(answer.reg(), Immediate(kSmiTagMask));
+      deferred->enter()->Branch(not_zero, &answer, not_taken);
+      deferred->exit()->Bind(&answer);
+      frame_->Push(&answer);
       break;
     }
 
     case Token::SAR: {
       if (reversed) {
-        frame_->EmitPop(eax);
-        frame_->EmitPush(Immediate(value));
-        frame_->EmitPush(eax);
+        Result top = frame_->Pop();
+        frame_->Push(value);
+        frame_->Push(&top);
         GenericBinaryOperation(op, type, overwrite_mode);
       } else {
+        VirtualFrame::SpilledScope spilled_scope(this);
         int shift_value = int_value & 0x1f;  // only least significant 5 bits
         DeferredCode* deferred =
           new DeferredInlinedSmiOperation(this, Token::SAR, shift_value,
@@ -1055,11 +1071,12 @@
 
     case Token::SHR: {
       if (reversed) {
-        frame_->EmitPop(eax);
-        frame_->EmitPush(Immediate(value));
-        frame_->EmitPush(eax);
+        Result top = frame_->Pop();
+        frame_->Push(value);
+        frame_->Push(&top);
         GenericBinaryOperation(op, type, overwrite_mode);
       } else {
+        VirtualFrame::SpilledScope spilled_scope(this);
         int shift_value = int_value & 0x1f;  // only least significant 5 bits
         DeferredCode* deferred =
         new DeferredInlinedSmiOperation(this, Token::SHR, shift_value,
@@ -1082,6 +1099,7 @@
     }
 
     case Token::SHL: {
+      VirtualFrame::SpilledScope spilled_scope(this);
       if (reversed) {
         frame_->EmitPop(eax);
         frame_->EmitPush(Immediate(value));
@@ -1113,6 +1131,7 @@
     case Token::BIT_OR:
     case Token::BIT_XOR:
     case Token::BIT_AND: {
+      VirtualFrame::SpilledScope spilled_scope(this);
       DeferredCode* deferred = NULL;
       if (!reversed) {
         deferred =  new DeferredInlinedSmiOperation(this, op, int_value,
@@ -1139,11 +1158,11 @@
 
     default: {
       if (!reversed) {

[Kevin Millikin (Chromium), 2009/01/23 08:49:25]

Reverse the arms of the if?

[William Hesse, 2012/05/07 09:35:10]

Done.

-        frame_->EmitPush(Immediate(value));
+        frame_->Push(value);
       } else {
-        frame_->EmitPop(eax);
-        frame_->EmitPush(Immediate(value));
-        frame_->EmitPush(eax);
+        Result top = frame_->Pop();
+        frame_->Push(value);
+        frame_->Push(&top);
       }
       GenericBinaryOperation(op, type, overwrite_mode);
       break;