Move SpilledFrame inside cases of SmiOperation....

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 171 matching lines @@
     }
 
     if (scope_->num_heap_slots() > 0) {
       Comment cmnt(masm_, "[ allocate local context");
       // Allocate local context.
       // Get outer context and create a new context based on it.
       frame_->PushFunction();
       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();
       }
       // Update context local.
       frame_->SaveContextRegister();
     }
 
     // TODO(1241774): Improve this code:
     // 1) only needed if we have a context
     // 2) no need to recompute context ptr every single time
@@ skipping 640 matching lines @@
 
  private:
   Token::Value op_;
   int value_;
   OverwriteMode overwrite_mode_;
 };
 
 
 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");
   }
 
   virtual void Generate() {

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

I also don't care for these inline-looking Generate functions in the deferred
code classes.  They can't be inlined at the call site anyway.  Feel free to move
it out and right after the class declaration while you're cleaning up this code.

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

Done.

     Result arg(generator());
     enter()->Bind(&arg);
     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");
   }
 
   virtual void Generate() {
     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);
     exit()->Jump(&arg);
   }
 
  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_;
 };
 
 
 class DeferredInlinedSmiSubReversed: public DeferredCode {
  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_;
 };
 
 
 void CodeGenerator::SmiOperation(Token::Value op,
                                  StaticType* type,
                                  Handle<Object> value,
                                  bool reversed,
                                  OverwriteMode overwrite_mode) {
   // NOTE: This is an attempt to inline (a bit) more of the code for
   // some possible smi operations (like + and -) when (at least) one
   // of the operands is a literal smi. With this optimization, the
   // performance of the system is increased by ~15%, and the generated
   // code size is increased by ~1% (measured on a combination of
   // different benchmarks).
 
   // 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();
       operand.ToRegister();
       frame_->Spill(operand.reg());
       __ add(Operand(operand.reg()), Immediate(value));
       deferred->enter()->Branch(overflow, &operand, not_taken);
       __ test(Operand(operand.reg()), Immediate(kSmiTagMask));
       deferred->enter()->Branch(not_zero, &operand, not_taken);
       deferred->exit()->Bind(&operand);
       frame_->Push(&operand);
       break;
     }
 
     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,
                                           overwrite_mode);
         frame_->EmitPop(eax);
         __ test(eax, Immediate(kSmiTagMask));
         deferred->enter()->Branch(not_zero, not_taken);
         __ sar(eax, shift_value);
         __ and_(eax, ~kSmiTagMask);
         deferred->exit()->Bind();
         frame_->EmitPush(eax);
       }
       break;
     }
 
     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,
                                         overwrite_mode);
         frame_->EmitPop(eax);
         __ test(eax, Immediate(kSmiTagMask));
         __ mov(ebx, Operand(eax));
         deferred->enter()->Branch(not_zero, not_taken);
         __ sar(ebx, kSmiTagSize);
         __ shr(ebx, shift_value);
         __ test(ebx, Immediate(0xc0000000));
         deferred->enter()->Branch(not_zero, not_taken);
         // tag result and store it in TOS (eax)
         ASSERT(kSmiTagSize == times_2);  // adjust code if not the case
         __ lea(eax, Operand(ebx, ebx, times_1, kSmiTag));
         deferred->exit()->Bind();
         frame_->EmitPush(eax);
       }
       break;
     }
 
     case Token::SHL: {
+      VirtualFrame::SpilledScope spilled_scope(this);
       if (reversed) {
         frame_->EmitPop(eax);
         frame_->EmitPush(Immediate(value));
         frame_->EmitPush(eax);
         GenericBinaryOperation(op, type, overwrite_mode);
       } else {
         int shift_value = int_value & 0x1f;  // only least significant 5 bits
         DeferredCode* deferred =
         new DeferredInlinedSmiOperation(this, Token::SHL, shift_value,
                                         overwrite_mode);
@@ skipping 11 matching lines @@
         __ lea(eax, Operand(ebx, ebx, times_1, kSmiTag));
         deferred->exit()->Bind();
         frame_->EmitPush(eax);
       }
       break;
     }
 
     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,
                                                     overwrite_mode);
       } else {
         deferred = new DeferredInlinedSmiOperationReversed(this, op, int_value,
                                                            overwrite_mode);
       }
       frame_->EmitPop(eax);
       __ test(eax, Immediate(kSmiTagMask));
       deferred->enter()->Branch(not_zero, not_taken);
       if (op == Token::BIT_AND) {
         __ and_(Operand(eax), Immediate(value));
       } else if (op == Token::BIT_XOR) {
         __ xor_(Operand(eax), Immediate(value));
       } else {
         ASSERT(op == Token::BIT_OR);
         __ or_(Operand(eax), Immediate(value));
       }
       deferred->exit()->Bind();
       frame_->EmitPush(eax);
       break;
     }
 
     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;
     }
   }
 }
 
 
 class CompareStub: public CodeStub {
  public:
@@ skipping 4952 matching lines @@
 
   // Slow-case: Go through the JavaScript implementation.
   __ bind(&slow);
   __ InvokeBuiltin(Builtins::INSTANCE_OF, JUMP_FUNCTION);
 }
 
 
 #undef __
 
 } }  // namespace v8::internal