Factor VirtualFrame::MergeTo() into three functions, for...

Factor VirtualFrame::MergeTo() into three functions, for
ease of reading and debugging.


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

[William Hesse, 2008-12-16 15:41:40]

A tiny review for U!

[iposva, 2008-12-17 00:46:18]

First set of comments. -ip

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

http://codereview.chromium.org/14158/diff/1/2#newcode573
Line 573: // one can be popped, bet first we may have to adjust the stack
bet->but

http://codereview.chromium.org/14158/diff/1/2#newcode584
Line 584: Immediate((stack_pointer_ - i) * kPointerSize));
At this point stack_pointer_ is (i + 1). Thus stack_pointer_ - i == (i + 1) - i
== 1 is the value being added to esp. I do not think this is the intention. Are
you sure this code was tested? I can imagine that it is pretty hard to generate
such a case which makes it really mandatory to ensure that this code works.
Otherwise you can run for years with such a bug in the code generator without
ever finding out about it.

More comments about your intentions would probably let you come to the same
conclusion while writing the code. Alternatively you could be less frugal with
your local variables and make your intentions explicit in the code (aka comments
get stale, code at least has to compile):

int new_stack_pointer = i + 1;
ASSERT((stack_pointer_ - new_stack_pointer) > 0);  // I know this assertion is a
duplicate of the above if check but it documents the precondition for the add
here and it ensures that the if check does not get changed.
__ add(Operand(esp), Immediate((stack_pointer_ - new_stack_pointer) *
kPointerSize);
stack_pointer_ = new_stack_pointer;

http://codereview.chromium.org/14158/diff/1/2#newcode601
Line 601: }
Unless I am missing something about merging frames, both stack_pointer_ values
should be equal at this point. Please assert that, alternatively this and
everything else should be asserted in MergeTo().

[William Hesse, 2008-12-17 08:45:12]

Made changes to moved code in MergeMoveMemoryToRegister, seemed to be off by
one.  Passes V8 and Mozilla tests, but
it passed them before, too.

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

http://codereview.chromium.org/14158/diff/1/2#newcode584
Line 584: Immediate((stack_pointer_ - i) * kPointerSize));
On 2008/12/17 00:46:18, iposva wrote:
> At this point stack_pointer_ is (i + 1). Thus stack_pointer_ - i == (i + 1) -
i
> == 1 is the value being added to esp. I do not think this is the intention.
Are
> you sure this code was tested? I can imagine that it is pretty hard to
generate
> such a case which makes it really mandatory to ensure that this code works.
> Otherwise you can run for years with such a bug in the code generator without
> ever finding out about it.
> 
> More comments about your intentions would probably let you come to the same
> conclusion while writing the code. Alternatively you could be less frugal with
> your local variables and make your intentions explicit in the code (aka
comments
> get stale, code at least has to compile):
> 
> int new_stack_pointer = i + 1;
> ASSERT((stack_pointer_ - new_stack_pointer) > 0);  // I know this assertion is
a
> duplicate of the above if check but it documents the precondition for the add
> here and it ensures that the if check does not get changed.
> __ add(Operand(esp), Immediate((stack_pointer_ - new_stack_pointer) *
> kPointerSize);
> stack_pointer_ = new_stack_pointer;
> 
> 

Amount added was also wrong.  More changes made.
Seemed to have a pervasive off-by-one error.
Needs review.

http://codereview.chromium.org/14158/diff/1/2#newcode601
Line 601: }
On 2008/12/17 00:46:18, iposva wrote:
> Unless I am missing something about merging frames, both stack_pointer_ values
> should be equal at this point. Please assert that, alternatively this and
> everything else should be asserted in MergeTo().

Everything is checked in MergeTo(), and the checking code
has been factored in a separate change.

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

Yeah, there was an off by one error in that code (and the instruction should
have been emitted before the state update, too).  Thanks for catching it and
fixing it.

http://codereview.chromium.org/14158/diff/5/205
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/14158/diff/5/205#newcode576
Line 576: int new_stack_pointer = i;  // Before a final pop.
I don't agree that this really makes the code clearer.  new_stack_pointer_ is
just i in a code block where i doesn't change.  I would probably just make the
comment immediately above clearer, that we are adjusting the stack pointer to be
equal to i.

http://codereview.chromium.org/14158/diff/5/205#newcode584
Line 584: Immediate((stack_pointer_ - new_stack_pointer) * kPointerSize));
If we keep the name new_stack_pointer_, we should probably name some part of
this long expresstion (stack_pointer_ - new_stack_pointer) * kPointerSize.  In
any case, the arguments should be aligned.

http://codereview.chromium.org/14158/diff/5/205#newcode595
Line 595: SyncRange(stack_pointer_ + 1, i);
I actually don't mind syncing zero-sized (or even negative) ranges since it does
the right thing.  The simplest way to state the precondition for SyncElementAt
is that everything between the stack pointer and the element (exclusive on both
ends) must be synced, even if that range includes no elements.

However, if we change it here, we should probably change it everywhere.

[William Hesse, 2008-12-17 10:16:39]

http://codereview.chromium.org/14158/diff/5/205
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/14158/diff/5/205#newcode576
Line 576: int new_stack_pointer = i;  // Before a final pop.
On 2008/12/17 09:50:12, Kevin Millikin wrote:
> I don't agree that this really makes the code clearer.  new_stack_pointer_ is
> just i in a code block where i doesn't change.  I would probably just make the
> comment immediately above clearer, that we are adjusting the stack pointer to
be
> equal to i.

Done.

http://codereview.chromium.org/14158/diff/5/205#newcode584
Line 584: Immediate((stack_pointer_ - new_stack_pointer) * kPointerSize));
On 2008/12/17 09:50:12, Kevin Millikin wrote:
> If we keep the name new_stack_pointer_, we should probably name some part of
> this long expresstion (stack_pointer_ - new_stack_pointer) * kPointerSize.  In
> any case, the arguments should be aligned.

Done.

http://codereview.chromium.org/14158/diff/5/205#newcode595
Line 595: SyncRange(stack_pointer_ + 1, i);
On 2008/12/17 09:50:12, Kevin Millikin wrote:
> I actually don't mind syncing zero-sized (or even negative) ranges since it
does
> the right thing.  The simplest way to state the precondition for SyncElementAt
> is that everything between the stack pointer and the element (exclusive on
both
> ends) must be synced, even if that range includes no elements.
> 
> However, if we change it here, we should probably change it everywhere.

I don't understand why we need to change it other places than here.  All that is
changing is the check if the range is empty.  I know it does the right thing,
but I think it takes thought to understand why the limit on the test is
different than the limit of the range.

[Kevin Millikin (Chromium), 2008-12-17 11:52:53]

http://codereview.chromium.org/14158/diff/5/205
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/14158/diff/5/205#newcode595
Line 595: SyncRange(stack_pointer_ + 1, i);
On 2008/12/17 10:16:39, William Hesse wrote:
> On 2008/12/17 09:50:12, Kevin Millikin wrote:
> > I actually don't mind syncing zero-sized (or even negative) ranges since it
> does
> > the right thing.  The simplest way to state the precondition for
SyncElementAt
> > is that everything between the stack pointer and the element (exclusive on
> both
> > ends) must be synced, even if that range includes no elements.
> > 
> > However, if we change it here, we should probably change it everywhere.
> 
> I don't understand why we need to change it other places than here.  All that
is
> changing is the check if the range is empty.  I know it does the right thing,
> but I think it takes thought to understand why the limit on the test is
> different than the limit of the range.

I just meant that the test should be (i > stack_pointer + 1) everywhere, but I
see that it already is at the only other current call site, so disregard.

We should consider making SyncElementAt responsible for allocating the space and
renaming the implementation (which assumes the space) to RawSyncElementAt.

[William Hesse, 2008-12-17 12:17:22]

I looked at the functions involved, and some comments seem wrong.  I'm not sure
why the names need changing.  Raw seems to mean not updating register counts,
rather than not checking for space.
Can I submit the CL as is?

http://codereview.chromium.org/14158/diff/5/205
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/14158/diff/5/205#newcode169
Line 169: // Clear the dirty bit for the element at a given index.  This
requires
This function only ever writes one dirty element to the frame, so the plural in
the comment is misleading.  Can be called with 0 <= index <= stack_pointer_ + 1.

http://codereview.chromium.org/14158/diff/5/205#newcode258
Line 258: // Make the type of the element at a given index be MEMORY.  We can
only
This comment seems wrong.  We can call SpillElementAt on any
index in the frame, even above the sp.

http://codereview.chromium.org/14158/diff/5/205#newcode270
Line 270: void VirtualFrame::SyncRange(int begin, int end) {
I think we have a precondition here, not commented, that begin must be less than
or equal to sp + 1.

[William Hesse, 2008-12-17 13:24:16]

Comments updated and corrected, SyncElementAt renamed to RawSyncElement at,
tested, and comitted.