Remove spilled frame from some function entry code....

Remove spilled frame from some function entry code.
Remove spilled frame from AddSmiImmediate code.
Add Push(Smi*).

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

[William Hesse, 2009-01-21 14:36:05]

Some small changes, setting the direction for further changes.

[Kevin Millikin (Chromium), 2009-01-21 15:09:58]

It basically looks right but I have some picky comments.

http://codereview.chromium.org/18616/diff/1/4
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/18616/diff/1/4#newcode192
Line 192: frame_->SpillAll();
I'm not sure why we spill here.  Perhaps a comment explaining why it's
necessary?

http://codereview.chromium.org/18616/diff/1/4#newcode196
Line 196: context.Unuse();
It's weird that context's lifetime is different depending on whether kDebug is
set or not, but I guess it's OK.

http://codereview.chromium.org/18616/diff/1/4#newcode865
Line 865: Result arg(generator());
"arg" isn't a good name since it's multiplexed for things that aren't the
argument.  Use a more generic name ("result") if you want to reuse the same
result, or just introduce another one.

http://codereview.chromium.org/18616/diff/1/4#newcode868
Line 868: generator()->frame()->Spill(arg.reg());  // Should not be needed.
The comment is strange.  Do you mean that it is not actually needed (why is it
there?)?  Do you mean that it is actually needed but it is possible to eliminate
that need (if so, make it a TODO or just do it).  Do you mean that it is a
precondition (then ASSERT it and structure the caller so it's true)?  Do you
mean that you don't know if it's needed or not?

http://codereview.chromium.org/18616/diff/1/4#newcode870
Line 870: Immediate immediate(Smi::FromInt(value_));
Since immediate is only used once, why not inline it?

http://codereview.chromium.org/18616/diff/1/4#newcode1011
Line 1011: frame_->Push(&operand);
This code works because virtual frame operations all work if the frame happens
to be spilled (they just don't preserve a spilled frame).  However, it would be
more explicit to push the spilled scope down into the other arms of the switch,
especially since we are using spilled scopes primarily to mark code which has
not been updated.

http://codereview.chromium.org/18616/diff/1/3
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/18616/diff/1/3#newcode296
Line 296: // Push the address of the frame slot containing the receiver on the
frame.
Frame slots don't have addresses (well, they do but we don't rely on them).  I
think this should be "stack slot" or even simpler "receiver slot".

[William Hesse, 2009-01-22 11:49:11]

Comments addressed or explained.

http://codereview.chromium.org/18616/diff/1/4
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/18616/diff/1/4#newcode192
Line 192: frame_->SpillAll();
On 2009/01/21 15:09:59, Kevin Millikin wrote:
> I'm not sure why we spill here.  Perhaps a comment explaining why it's
> necessary?

It is needed for the breakpoint below.  If the spill is only on the breakpoint
branch, then the Bind below it will undo the spill, which is probably not worth
it.

I added a comment.

http://codereview.chromium.org/18616/diff/1/4#newcode868
Line 868: generator()->frame()->Spill(arg.reg());  // Should not be needed.
On 2009/01/21 15:09:59, Kevin Millikin wrote:
> The comment is strange.  Do you mean that it is not actually needed (why is it
> there?)?  Do you mean that it is actually needed but it is possible to
eliminate
> that need (if so, make it a TODO or just do it).  Do you mean that it is a
> precondition (then ASSERT it and structure the caller so it's true)?  Do you
> mean that you don't know if it's needed or not?

It is defensive programming.  It is currently not needed, but changes in the
caller could make it needed.  The comment is removed.
Done.

http://codereview.chromium.org/18616/diff/1/4#newcode870
Line 870: Immediate immediate(Smi::FromInt(value_));
On 2009/01/21 15:09:59, Kevin Millikin wrote:
> Since immediate is only used once, why not inline it?

Done.

http://codereview.chromium.org/18616/diff/1/4#newcode1011
Line 1011: frame_->Push(&operand);
On 2009/01/21 15:09:59, Kevin Millikin wrote:
> This code works because virtual frame operations all work if the frame happens
> to be spilled (they just don't preserve a spilled frame).  However, it would
be
> more explicit to push the spilled scope down into the other arms of the
switch,
> especially since we are using spilled scopes primarily to mark code which has
> not been updated.

The next changelist does this.

http://codereview.chromium.org/18616/diff/1/3
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/18616/diff/1/3#newcode296
Line 296: // Push the address of the frame slot containing the receiver on the
frame.
On 2009/01/21 15:09:59, Kevin Millikin wrote:
> Frame slots don't have addresses (well, they do but we don't rely on them).  I
> think this should be "stack slot" or even simpler "receiver slot".

Done.

[Kevin Millikin (Chromium), 2009-01-22 13:18:17]

LGTM.