Begin using the virtual frame for variable proxies and slots. Loading...

Begin using the virtual frame for variable proxies and slots.  Loading
a frame-allocated slot (ie, a frame-allocated local or parameter) to
the top of the frame causes it te be allocated to a register if it is
in memory, and then the register copied to the top of the frame.

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

[Kevin Millikin (Chromium), 2008-12-10 16:40:56]

This change affects variable references as rvalues.

If they're in memory, I chose to move them to a register to avoid the issue of
multiple virtual frame elements backed by the same in-memory location.  I
suspect that we will need them in a register quite soon anyway.

Instead, we have duplicated registers which will have copy on write semantics. 
We don't have to deal with that yet, because they are spilled quite soon after.

[iposva, 2008-12-11 07:59:36]

Mostly naming and readability comments.

But one thing that starts to be apparent to me is that we are missing a way to
get the frame element out of a virtual frame, which makes us use these very
thick APIs that should really be named LoadFrameSlotAtAndPushToTOS(). Let's
discuss a couple ideas on how to solve this in person.

-Ivan

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

http://codereview.chromium.org/11023/diff/201/401#newcode337
Line 337: // stay in the register.)
What is the reason to keep the lowest most register reference of a register
instead of the top-most?

http://codereview.chromium.org/11023/diff/201/401#newcode339
Line 339: FrameElement memory_element;
I find the fact that by default FrameElements are created as memory elements
confusing. I was looking at the use of memory_element below and wondered where
it was setup as a memory element. One solution would be to have a factory for
the frame elements instead of constructors, so that you can say
VirtualFrame::MemoryElement(). Maybe you have a better idea.

http://codereview.chromium.org/11023/diff/201/401#newcode397
Line 397: }
Alternatively instead of increasing the use count for this register again, you
could define a RawSpillElementAt(i) which does not deal with any use counts. It
could make the code a little bit more readable.

http://codereview.chromium.org/11023/diff/201/401#newcode595
Line 595: void VirtualFrame::LoadFrameSlotAt(int index) {
Shouldn't this be PushFrameSlotAt()? Essentially this is what it does.
Maybe I have done too much RISC assembly in my past, but Load to me indicates a
load into a register, but here it has a side-effect of also duplicating the
value on the top of the stack which is unclear from the function name.

http://codereview.chromium.org/11023/diff/201/401#newcode631
Line 631: void VirtualFrame::StoreToFrameSlotAt(int index) {
Note the assymetry from the above function? In this case Store... does not pop
from the top of the stack.

http://codereview.chromium.org/11023/diff/201/401#newcode639
Line 639: // as the frame top.
This comment is slightly misleading:and it is misplaced because the code
following right after it deals with the indexed slots register use count. At
least:  "is now" -> "will be"

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

http://codereview.chromium.org/11023/diff/201/402#newcode269
Line 269: void LoadLocalAt(int index) {
Same comments as in the .cc file. LoadLocalAt is asymmetric to StoreToLocalAt()
and it has a side effect in that it pushes the value that is not clear from the
function name.

http://codereview.chromium.org/11023/diff/201/402#newcode293
Line 293: void LoadParameterAt(int index) {
ditto.

http://codereview.chromium.org/11023/diff/201/402#newcode451
Line 451: void LoadFrameSlotAt(int index);
ditto.

[Kevin Millikin (Chromium), 2008-12-11 08:32:04]

Yes, let's talk about it tonight (tomorrow morning).

FrameElements were always conceived as an internal utility class for the frame,
not something that would be exposed (and definitely not manipulated) from
outside the frame.  We have called the thing that can be manipulated from
outside the frame "Result"---but currently it has no connection to the frame at
all.

As for the naming ("Load..."), I agree with you.  However, in the context of the
existing code generator, "Load" already implicitly means "evaluate to the top of
the stack" where the cc register is a possible top of stack.  It may seem that
using it on something that is not an Expression is an abuse of terminology, but
we already use it that way, ie, in LoadGlobal.

As for the load/store asymmetry, we have been discovering frame operations as we
go along with converting the code generator.  The result is that the frame
abstraction is quite closely tailored to the needs of the V8 code generator.  We
*could* make store symmetrical with load and implement the current store as a
more RISCy duplicate followed by store, but then I'm concerned that we don't
actually need all that generality and may miss some chances to generate good
code (effectively require peephole optimization of virtual frame operations).

[Kevin Millikin (Chromium), 2008-12-11 09:17:30]

Other than the naming issues, I've addressed your comments.

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

http://codereview.chromium.org/11023/diff/201/401#newcode337
Line 337: // stay in the register.)
On 2008/12/11 07:59:36, iposva wrote:
> What is the reason to keep the lowest most register reference of a register
> instead of the top-most?

Ease of implementation for now.  Once we start tuning on benchmarks, I'm sure we
will discover that it's exactly the wrong heuristic.

http://codereview.chromium.org/11023/diff/201/401#newcode339
Line 339: FrameElement memory_element;
On 2008/12/11 07:59:36, iposva wrote:
> I find the fact that by default FrameElements are created as memory elements
> confusing. I was looking at the use of memory_element below and wondered where
> it was setup as a memory element. One solution would be to have a factory for
> the frame elements instead of constructors, so that you can say
> VirtualFrame::MemoryElement(). Maybe you have a better idea.

Factory methods are probably cleanest.  I'll do that as a separate change.

http://codereview.chromium.org/11023/diff/201/401#newcode397
Line 397: }
On 2008/12/11 07:59:36, iposva wrote:
> Alternatively instead of increasing the use count for this register again, you
> could define a RawSpillElementAt(i) which does not deal with any use counts.
It
> could make the code a little bit more readable.
Done.

http://codereview.chromium.org/11023/diff/201/401#newcode639
Line 639: // as the frame top.
On 2008/12/11 07:59:36, iposva wrote:
> This comment is slightly misleading:and it is misplaced because the code
> following right after it deals with the indexed slots register use count. At
> least:  "is now" -> "will be"

Fixed.

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

http://codereview.chromium.org/11023/diff/201/402#newcode269
Line 269: void LoadLocalAt(int index) {
On 2008/12/11 07:59:36, iposva wrote:
> Same comments as in the .cc file. LoadLocalAt is asymmetric to
StoreToLocalAt()
> and it has a side effect in that it pushes the value that is not clear from
the
> function name.

I agree about the naming issue.  Perhaps "fetch" is better?  I will leave it as
is for now and we can discuss it in person.

[William Hesse, 2008-12-11 12:36:25]

Looks good to me, with Ivan's comments taken care of.

[iposva, 2008-12-11 18:50:16]

I do not see the fixes you say you made. Have you uploaded the latest version?
-Ivan

[Kevin Millikin (Chromium), 2008-12-11 20:17:33]

Oops.  Uploaded.

[iposva, 2008-12-11 21:28:04]

Better now. I know we will have to tweak the function names more, so commit what
you have right now.