First step toward allowing constants to appear in the virtual frame...

First step toward allowing constants to appear in the virtual frame
without being materialized on the actual frame.

Currently constants are materialized almost immediately: as soon as we
call the runtime; push any non-constant value on top of them; jump,
branch, or bind a jump target; or generate code for any AST node.

Committed: http://code.google.com/p/v8/source/detail?r=788

[William Hesse, 2008-11-18 16:37:34]

Looks good to me, but I have some questions.

http://codereview.chromium.org/11232/diff/201/13
File src/codegen-ia32.cc (left):

http://codereview.chromium.org/11232/diff/201/13#oldcode203
Line 203: // Save the arguments object pointer, if any.
What is this change?

http://codereview.chromium.org/11232/diff/201/10
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/11232/diff/201/10#newcode49
Line 49: frame_pointer_(kIllegalIndex) {
In this case, is IllegalIndex actually the accurate value of frame_pointer, not
a sign of illegality?
Perhaps illegalIndex should not be -1, but should be a less
likely negative value.

http://codereview.chromium.org/11232/diff/201/10#newcode56
Line 56: // When cloned, a frame is a deep copy of the original.
How deep?  Are heap objects cloned?

http://codereview.chromium.org/11232/diff/201/10#newcode77
Line 77: ASSERT(stack_pointer_ == elements_.length() - 1);
Isn't this too strong a precondition for a useful Adjust()?  Won't you want to
use Adjust on more complex frames?

http://codereview.chromium.org/11232/diff/201/10#newcode109
Line 109: __ mov(Operand(ebp, fp_relative(i)),
Immediate(elements_[i].handle()));
Is the splitting into two 16-bit quantities inside mov?

http://codereview.chromium.org/11232/diff/201/11
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/11232/diff/201/11#newcode52
Line 52: Type type() const { return static_cast<Type>(type_ & ~0x1); }
Name the dirty flag mask.  Unless you are also storing SMIs in this word, I
would use a higher bit for the flag, and store the types in the low bits, rather
than using only even type IDs.

http://codereview.chromium.org/11232/diff/201/11#newcode66
Line 66: Handle<Object> handle() const { return Handle<Object>(data_.handle_); }
Put asserts here to assert it is a type that has handle data?

http://codereview.chromium.org/11232/diff/201/11#newcode76
Line 76: } data_;
ASSERT( sizeof data_  == sizeof what? )

http://codereview.chromium.org/11232/diff/201/11#newcode254
Line 254: void PrepareCall(int count);
PrepareForCall?

[Kevin Millikin (Chromium), 2008-11-18 19:12:06]

http://codereview.chromium.org/11232/diff/201/13
File src/codegen-ia32.cc (left):

http://codereview.chromium.org/11232/diff/201/13#oldcode203
Line 203: // Save the arguments object pointer, if any.
On 2008/11/18 16:37:34, William Hesse wrote:
> What is this change?

As far as I can tell, the only reason to save ecx here is because it will be
clobbered by SlotOperand and RecordWrite.  All the gymnastics with saving ecx
will go away as soon as we can support registers in the virtual frame, anyway.

http://codereview.chromium.org/11232/diff/201/10
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/11232/diff/201/10#newcode49
Line 49: frame_pointer_(kIllegalIndex) {
On 2008/11/18 16:37:34, William Hesse wrote:
> In this case, is IllegalIndex actually the accurate value of frame_pointer,
not
> a sign of illegality?
> Perhaps illegalIndex should not be -1, but should be a less
> likely negative value.

We don't know what the frame pointer is until we set it in Enter().

http://codereview.chromium.org/11232/diff/201/10#newcode77
Line 77: ASSERT(stack_pointer_ == elements_.length() - 1);
On 2008/11/18 16:37:34, William Hesse wrote:
> Isn't this too strong a precondition for a useful Adjust()?  Won't you want to
> use Adjust on more complex frames?

I think it's right.  The intent of "Adjust" is that it doesn't generate code but
only fixes up the frame to reflect code already (or about to be) generated.

If the sp is not at the top of the virtual frame when adjust is called, the it's
not safe to have someone else push on the stack.

http://codereview.chromium.org/11232/diff/201/10#newcode109
Line 109: __ mov(Operand(ebp, fp_relative(i)),
Immediate(elements_[i].handle()));
On 2008/11/18 16:37:34, William Hesse wrote:
> Is the splitting into two 16-bit quantities inside mov?

There is no splitting here (yet).  The only constants right now are not from
source code.

http://codereview.chromium.org/11232/diff/201/11
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/11232/diff/201/11#newcode52
Line 52: Type type() const { return static_cast<Type>(type_ & ~0x1); }
On 2008/11/18 16:37:34, William Hesse wrote:
> Name the dirty flag mask.  Unless you are also storing SMIs in this word, I
> would use a higher bit for the flag, and store the types in the low bits,
rather
> than using only even type IDs.

OK.

http://codereview.chromium.org/11232/diff/201/11#newcode66
Line 66: Handle<Object> handle() const { return Handle<Object>(data_.handle_); }
On 2008/11/18 16:37:34, William Hesse wrote:
> Put asserts here to assert it is a type that has handle data?

OK.

http://codereview.chromium.org/11232/diff/201/11#newcode254
Line 254: void PrepareCall(int count);
On 2008/11/18 16:37:34, William Hesse wrote:
> PrepareForCall?
> 

OK.