Experimental: initial simple support for registers in the virtual...

v8/src/virtual-frame-ia32.cc

Index: v8/src/virtual-frame-ia32.cc
===================================================================
--- v8/src/virtual-frame-ia32.cc	(revision 792)
+++ v8/src/virtual-frame-ia32.cc	(working copy)
@@ -99,25 +99,46 @@
 }
 
 
-void VirtualFrame::SpillAll() {
-  int i = 0;
+void VirtualFrame::SyncElementAt(int index) {

[iposva, 2008/11/20 05:49:35]

Comment please.

+  FrameElement element = elements_[index];
 
-  // Spill dirty constants below the stack pointer.
-  for (; i <= stack_pointer_; i++) {
-    if (elements_[i].type() == FrameElement::CONSTANT &&
-        elements_[i].is_dirty()) {
-      __ mov(Operand(ebp, fp_relative(i)), Immediate(elements_[i].handle()));
-      elements_[i] = FrameElement();  // The element is now in memory.
+  if (index <= stack_pointer_ && element.is_dirty()) {
+    // Write elements below the stack pointer to their (already allocated)
+    // actual frame location.
+    if (element.is_constant()) {
+      __ Set(Operand(ebp, fp_relative(index)), Immediate(element.handle()));
+    } else {
+      ASSERT(element.is_register());
+      __ mov(Operand(ebp, fp_relative(index)), element.reg());
     }
+
+  } else if (index > stack_pointer_) {
+    // Push elements above the stack pointer to allocate space and sync
+    // them.  Space should have already been allocated in the actual frame
+    // for all the elements below this one.
+    ASSERT(index == stack_pointer_ - 1);

[iposva, 2008/11/20 05:49:35]

I would expect that elements above the stack pointer are also automatically
marked as dirty. They have to be, no?

That would simplify your logic here as well:
if (element.dirty()) {
  if (index <= stackpointer) {
    ...
  } else {
    ASSERT(index == stack_pointer_ -1 );
    ...
  }
}

I also find the math in the ASSERT rather strange: index is greater than
stack_pointer_, but then you assert that index is equal to stack_pointer minus
one. This seems like a static assertion failure to me. Are you sure this code
has been exercised?

[Kevin Millikin (Chromium), 2008/11/20 08:28:03]

Yes: elements above the stack pointer are always dirty, so your simplified code
is better.

Yes: the assertion is just wrong.  I meant "+ 1" or "- 1" on the other side or
something.

+    if (element.is_constant()) {
+      stack_pointer_++;
+      __ push(Immediate(element.handle()));
+    } else {
+      ASSERT(element.is_register());
+      stack_pointer_++;

[iposva, 2008/11/20 05:49:35]

The stack_pointer_ update should be factored out of the if statement.

[Kevin Millikin (Chromium), 2008/11/20 08:28:03]

OK.

+      __ push(element.reg());
+    }
   }
+}
 
-  // Spill all constants above the stack pointer.
-  for (; i < elements_.length(); i++) {
-    ASSERT(elements_[i].type() == FrameElement::CONSTANT);
-    ASSERT(elements_[i].is_dirty());
-    stack_pointer_++;
-    __ push(Immediate(elements_[i].handle()));
-    elements_[i] = FrameElement();  // The element is now in memory.
+
+void VirtualFrame::SpillElementAt(int index) {

[iposva, 2008/11/20 05:49:35]

Comment please.

Especially since the distinction between synching and spilling an element is
relatively small but important.

+  SyncElementAt(index);
+  // The element is now in memory.
+  elements_[index] = FrameElement();
+}
+
+
+void VirtualFrame::SpillAll() {
+  for (int i = 0; i < elements_.length(); i++) {
+    SpillElementAt(i);
   }
 }
 
@@ -125,25 +146,34 @@
 void VirtualFrame::PrepareForCall(int frame_arg_count) {
   ASSERT(height() >= frame_arg_count);
 
-  // The only non-memory elements of the frame are constants.  Push all of
-  // them above the stack pointer to allocate space for them and to ensure
-  // the arguments are flushed to memory.
+  // Below the stack pointer, spill all registers.
+  for (int i = 0; i <= stack_pointer_; i++) {
+    if (elements_[i].is_register()) {
+      SpillElementAt(i);
+    }
+  }
+
+  // Above the stack pointer, spill registers and sync everything else (ie,
+  // constants).
   for (int i = stack_pointer_ + 1; i < elements_.length(); i++) {
-    ASSERT(elements_[i].type() == FrameElement::CONSTANT);
-    ASSERT(elements_[i].is_dirty());
-    stack_pointer_++;
-    elements_[i].clear_dirty();
-    __ push(Immediate(elements_[i].handle()));
+    if (elements_[i].is_register()) {
+      SpillElementAt(i);
+    } else {
+      SyncElementAt(i);
+    }
   }
 
-  // Forget the ones that will be popped by the call.
+  // Forget the frame elements that will be popped by the call.
   Forget(frame_arg_count);
 }
 
 
 void VirtualFrame::EnsureMergable() {
   // We cannot merge to a frame that has constants as elements, because an
-  // arbitrary frame may not have constants in those locations.
+  // arbitrary frame might not have constants in those locations.
+  //
+  // We cannot merge to a frame that has registers as elements because we
+  // haven't implemented merging for such frames yet.
   SpillAll();
 }
 
@@ -155,9 +185,11 @@
   ASSERT(local_count_ == expected->local_count_);
   ASSERT(frame_pointer_ == expected->frame_pointer_);
 
-  // The expected frame is one we can merge to (ie, currently that means
-  // that all elements are in memory).  The only thing we need to do to
-  // merge is make this one mergable too.
+  // Mergable frames do not have constants and they do not (currently) have
+  // registers.  They are always fully spilled, so the only thing needed to
+  // make this frame match the expected one is to spill everything.
+  //
+  // TODO(): Implement a non-stupid way of merging frames.
   SpillAll();
 
   ASSERT(stack_pointer_ == expected->stack_pointer_);
@@ -172,11 +204,21 @@
   __ mov(ebp, Operand(esp));
 
   // Store the context and the function in the frame.
-  EmitPush(esi);
-  EmitPush(edi);
+  FrameElement context(esi);
+  context.clear_dirty();
+  elements_.Add(context);
+  stack_pointer_++;
+  __ push(esi);
 
+  FrameElement function(edi);
+  function.clear_dirty();
+  elements_.Add(function);
+  stack_pointer_++;
+  __ push(edi);
+
   // Clear the function slot when generating debug code.
   if (FLAG_debug_code) {
+    SpillElementAt(stack_pointer_);
     __ Set(edi, Immediate(reinterpret_cast<int>(kZapValue)));
   }
 }
@@ -206,8 +248,19 @@
 void VirtualFrame::AllocateStackSlots(int count) {
   ASSERT(height() == 0);
   local_count_ = count;
-  for (int i = 0; i < count; i++) {
-    elements_.Add(FrameElement(Factory::undefined_value()));
+
+  if (count > 0) {
+    Comment cmnt(masm_, "[ Allocate space for locals");
+    // The locals are constants (the undefined value), but we sync them with
+    // the actual frame to allocate space for spilling them.
+    FrameElement initial_value(Factory::undefined_value());
+    initial_value.clear_dirty();
+    __ Set(eax, Immediate(Factory::undefined_value()));
+    for (int i = 0; i < count; i++) {
+      elements_.Add(initial_value);
+      stack_pointer_++;
+      __ push(eax);
+    }
   }
 }