Begin using the virtual frame for assignment statements of the form:...

Begin using the virtual frame for assignment statements of the form:

  <local> = <literal>

Literals are pushed on the frame as virtual elements.  Assignment of a
virtual element to a frame slot (eg, local or parameter) makes that slot
virtual too.  The above code compiles into a single move of a constant to
memory, and that move is actually only caused by spilling the frame.

Visiting expressions is guarded by spilling the frame to keep everything
spilled in code that assumes it.  That's ugly but temporary.

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

[Kevin Millikin (Chromium), 2008-11-28 08:56:05]

Without allocating locals to registers, we are not yet doing better than the
peephole optimizer.

Ivan, I'd still appreciate your comments but I will probably submit this before
the end of the day.

[William Hesse, 2008-11-28 10:32:45]

Aside from my comments, it looks good.  But Ivan should look at it, I think.

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

http://codereview.chromium.org/10993/diff/1/5#newcode2600
Line 2600: // Smis are loaded in two steps via a temporary register.
Is now the time for special-casing small literals, and 0?
Load 0 by xor, and load literals between -255 and 255, or 16-bit literals,
specially?  I think this would hit a majority of code literals.

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

http://codereview.chromium.org/10993/diff/1/3#newcode350
Line 350: void VirtualFrame::StoreToFrameSlotAt(int index) {
So this changelist enables the register and memory types, and also the constant
type, since there is no extra processing for the constant type?  Should it be
commented that this correctly handles constants, since there is no explicit
mention of it in the code?

http://codereview.chromium.org/10993/diff/1/3#newcode358
Line 358: elements_[index] = top;
Don't we need to unuse elements_[index] if it is a register here?

http://codereview.chromium.org/10993/diff/1/3#newcode363
Line 363: ASSERT(!tmp.is(no_reg));
Will we actually need error handling code in the future, to deal with the case
that the allocator fails?
Or did we decide that the allocator must always be able to succeed, by spilling,
because we will never have all regs externally used?

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

http://codereview.chromium.org/10993/diff/1/4#newcode73
Line 73: type_ = (type_ & ~SyncField::mask()) | SyncField::encode(SYNCED);
Why isn't there a set_field accessor for bit fields?  Is this the way we always
set bit fields?

[Kevin Millikin (Chromium), 2008-11-28 12:19:07]

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

http://codereview.chromium.org/10993/diff/1/5#newcode2600
Line 2600: // Smis are loaded in two steps via a temporary register.
On 2008/11/28 10:32:45, William Hesse wrote:
> Is now the time for special-casing small literals, and 0?
> Load 0 by xor, and load literals between -255 and 255, or 16-bit literals,
> specially?  I think this would hit a majority of code literals.

It's only the non-"InlineSmi" literals that are loaded in two pieces---those
that don't fit in 16 bits.  Small ones hit the path to be pushed as a single
virtual frame element.

If we eventually spill literals in VirtualFrame, we use MacroAssembler::Set,
which takes care of using the xor trick for zero.

Ultimately, we can even split the literal up lazily, so if it gets
constant-folded there is no need.

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

http://codereview.chromium.org/10993/diff/1/3#newcode350
Line 350: void VirtualFrame::StoreToFrameSlotAt(int index) {
On 2008/11/28 10:32:45, William Hesse wrote:
> So this changelist enables the register and memory types, and also the
constant
> type, since there is no extra processing for the constant type?  Should it be
> commented that this correctly handles constants, since there is no explicit
> mention of it in the code?

It is expected to correctly handle constants.

http://codereview.chromium.org/10993/diff/1/3#newcode358
Line 358: elements_[index] = top;
On 2008/11/28 10:32:45, William Hesse wrote:
> Don't we need to unuse elements_[index] if it is a register here?

Yes.  Good catch.

http://codereview.chromium.org/10993/diff/1/3#newcode363
Line 363: ASSERT(!tmp.is(no_reg));
On 2008/11/28 10:32:45, William Hesse wrote:
> Will we actually need error handling code in the future, to deal with the case
> that the allocator fails?
> Or did we decide that the allocator must always be able to succeed, by
spilling,
> because we will never have all regs externally used?

We don't have a good way to handle that yet.  I'm trying to put these assertions
everywhere it could occur and we can address it when it does.

The only way it can happen is if we are in new-codegen code (using the frame)
and we have all registers referenced from outside the frame.  That doesn't
happen right now, because we never use more than one non-frame register (as a
temporary) yet.

Statically, we can guarantee that it can't happen, but I'm not sure what's the
best way to enforce that in the code other than assertions and good tests....

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

http://codereview.chromium.org/10993/diff/1/4#newcode73
Line 73: type_ = (type_ & ~SyncField::mask()) | SyncField::encode(SYNCED);
On 2008/11/28 10:32:45, William Hesse wrote:
> Why isn't there a set_field accessor for bit fields?  Is this the way we
always
> set bit fields?

I don't know why there isn't such an accessor.  It's not something we do very
often, I guess (we don't have a lot of mutable bit fields).

This seems like a safe way to set/clear it that doesn't rely on knowing which
other fields are packed in with it or which sense the flag is.

It's the same way we do it in AccessorInfo::set_property_attributes, for
instance.

[iposva, 2008-12-03 07:04:39]

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

http://codereview.chromium.org/10993/diff/1/5#newcode3112
Line 3112: frame_->SpillAll();
Is there a single call to Load in this file that is not followed by a
frame_->SpillAll()?
You should probably move the mandatory (?) spilling code into the Load function
and explain the reason for it in a good fat comment.

Style nit: we should use the accessor to get at the frame_ instead of using it
directly.

http://codereview.chromium.org/10993/diff/1/5#newcode4185
Line 4185: frame->SpillAll();
Could this SpillAll() be moved after the EmitPop(eax)? Then you could
potentially materialize the value for eax without having to go through memory
and then spill the remainder after EmitPop().

http://codereview.chromium.org/10993/diff/1/5#newcode4200
Line 4200: frame->SpillAll();
ditto.

[Kevin Millikin (Chromium), 2008-12-03 15:08:38]

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

http://codereview.chromium.org/10993/diff/1/5#newcode3112
Line 3112: frame_->SpillAll();
On 2008/12/03 07:04:40, iposva wrote:
> Is there a single call to Load in this file that is not followed by a
> frame_->SpillAll()?
> You should probably move the mandatory (?) spilling code into the Load
function
> and explain the reason for it in a good fat comment.

Actually there are two (but they are easy to miss).  one is in
VisitExpressionStatement where a value is discarded, which doesn't require a
fully spilled frame and emits no code for virtual TOS elements; and the other is
in VisitAssignment for the case of simple "=" assignments, where the RHS can
safely be a register or constant which is simply compiled to a memory move for
LHSs that are LOCAL or PARAMETER slots.

The calls to SpillAll are annoying, but temporary---ultimately they will all go
away (or be moved to more optimal places).

I don't mind using the accessor, but I'm not sure what it's buying us either.

http://codereview.chromium.org/10993/diff/1/5#newcode4185
Line 4185: frame->SpillAll();
On 2008/12/03 07:04:40, iposva wrote:
> Could this SpillAll() be moved after the EmitPop(eax)? Then you could
> potentially materialize the value for eax without having to go through memory
> and then spill the remainder after EmitPop().

Eventually when this code is reworked.  Right now the functions with "Emit" (ie,
EmitPush and EmitPop) in their names will actually emit those specific machine
instructions.  They do not do any magic with virtual frame elements, so they
require fully spilled frames.