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

v8/src/virtual-frame-ia32.cc

 // Copyright 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 81 matching lines @@
   ASSERT(stack_pointer_ == elements_.length() - 1);
   ASSERT(elements_.length() >= count);
 
   stack_pointer_ -= count;
   for (int i = 0; i < count; i++) {
     elements_.RemoveLast();
   }
 }
 
 
+void VirtualFrame::SyncElementAt(int index) {

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

Comment please.

+  FrameElement element = elements_[index];
+
+  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());
+    }
+  }
+}
+
+
+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() {
-  int i = 0;
-
-  // 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.
-    }
-  }
-
-  // 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.
+  for (int i = 0; i < elements_.length(); i++) {
+    SpillElementAt(i);
   }
 }
 
 
 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.
-  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()));
+  // Below the stack pointer, spill all registers.
+  for (int i = 0; i <= stack_pointer_; i++) {
+    if (elements_[i].is_register()) {
+      SpillElementAt(i);
+    }
   }
 
-  // Forget the ones that will be popped by the call.
+  // Above the stack pointer, spill registers and sync everything else (ie,
+  // constants).
+  for (int i = stack_pointer_ + 1; i < elements_.length(); i++) {
+    if (elements_[i].is_register()) {
+      SpillElementAt(i);
+    } else {
+      SyncElementAt(i);
+    }
+  }
+
+  // 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();
 }
 
 
 void VirtualFrame::MergeTo(VirtualFrame* expected) {
   ASSERT(masm_ == expected->masm_);
   ASSERT(elements_.length() == expected->elements_.length());
   ASSERT(parameter_count_ == expected->parameter_count_);
   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_);
 }
 
 
 void VirtualFrame::Enter() {
   Comment cmnt(masm_, "[ Enter JS frame");
   EmitPush(ebp);
 
   frame_pointer_ = stack_pointer_;
   __ 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)));
   }
 }
 
 
 void VirtualFrame::Exit() {
   Comment cmnt(masm_, "[ Exit JS frame");
   // Record the location of the JS exit code for patching when setting
   // break point.
   __ RecordJSReturn();
 
   // Avoid using the leave instruction here, because it is too
   // short. We need the return sequence to be a least the size of a
   // call instruction to support patching the exit code in the
   // debugger. See VisitReturnStatement for the full return sequence.
   __ mov(esp, Operand(ebp));
   stack_pointer_ = frame_pointer_;
   for (int i = elements_.length() - 1; i > stack_pointer_; i--) {
     elements_.RemoveLast();
   }
 
   frame_pointer_ = kIllegalIndex;
   EmitPop(ebp);
 }
 
 
 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);
+    }
   }
 }
 
 
 void VirtualFrame::PushTryHandler(HandlerType type) {
   // Grow the expression stack by handler size less two (the return address
   // is already pushed by a call instruction, and PushTryHandler from the
   // macro assembler will leave the top of stack in the eax register to be
   // pushed separately).
   Adjust(kHandlerSize - 2);
@@ skipping 92 matching lines @@
   ASSERT(stack_pointer_ == elements_.length() - 1);
   elements_.Add(FrameElement());
   stack_pointer_++;
   __ push(immediate);
 }
 
 
 #undef __
 
 } }  // namespace v8::internal