Change VirtualFrame::AdjustCopies to mark target as invalid.

Change VirtualFrame::AdjustCopies to mark target as invalid.
Change its name to VirtualFrame::InvalidateFrameSlot

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

[Kevin Millikin (Chromium), 2009-03-19 15:45:52]

The Use/Unuse protocol in AdjustCopies should be simplified, then it LGTM.

I have a couple of other comments that could be saved for another change.  I
think we still need more aggressive refactoring---there are too many places
where we end up doing almost the same thing (adjusting copies, unusing
registers, overwriting frame element).

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

http://codereview.chromium.org/50012/diff/1/2#newcode525
Line 525: __ mov(backing_reg, Operand(ebp, fp_relative(index)));
I think it's clearer to Use(backing_reg) just before or after this line rather
than unusing it in the register case and then calling use on it again in both
cases.  That way, there is a single use on the memory case, and no unuse/use on
the register case.

http://codereview.chromium.org/50012/diff/1/2#newcode535
Line 535: new_backing_element.set_sync();
You might consider just putting the creation of the new element and assignment
to elements_[i] in this conditional (making it an if/else).  If we're going to
test the sync flag, we might as well create it with the right one in the first
place.

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

http://codereview.chromium.org/50012/diff/1/3#newcode388
Line 388: Unuse(elements_[frame_index].reg());
It seems like we actually want to make (a suitably renamed) "AdjustCopies"
always invalidate the frame element at the argument index (unusing the
register), and we might be able to simplify this so we don't need the second
test?

I don't think we ever need the frame slot after AdjustCopies (that's the point),
so call it "InvalidateFrameSlotAt" or something.  It's a little weird to emit
code for a frame element marked invalid, so we could go farther and have
SetFrameSlotAt and pass it in invalid element in the one case where we
*actually* want it to be invalidated.  What do you think?

[William Hesse, 2009-03-20 13:48:16]

Changed name, added ARM changes.

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

http://codereview.chromium.org/50012/diff/1/2#newcode525
Line 525: __ mov(backing_reg, Operand(ebp, fp_relative(index)));
On 2009/03/19 15:45:52, Kevin Millikin wrote:
> I think it's clearer to Use(backing_reg) just before or after this line rather
> than unusing it in the register case and then calling use on it again in both
> cases.  That way, there is a single use on the memory case, and no unuse/use
on
> the register case.

Done.

http://codereview.chromium.org/50012/diff/1/2#newcode535
Line 535: new_backing_element.set_sync();
On 2009/03/19 15:45:52, Kevin Millikin wrote:
> You might consider just putting the creation of the new element and assignment
> to elements_[i] in this conditional (making it an if/else).  If we're going to
> test the sync flag, we might as well create it with the right one in the first
> place.

Done, and made much more direct - pass the sync value from elements_[i] to the
constructor.
Also optimized below - create an unsynced and a synced copy, rather than
continually setting and unsetting the sync bit.

[Kevin Millikin (Chromium), 2009-03-20 14:44:55]

It's a little confusing (probably because of the original code).  The biggest
thing is that I think "InvalidateFrameSlotAt" should just handle any kind of
frame element.

I think that makes it cleaner.  We can eventually replace direct assignments of
invalid elements with calls to this function (eg, when we Drop or Forget
elements, we have to do essentially all this work anyway).  And it gives us a
single point to hang "becoming invalid" behavior in the future.

http://codereview.chromium.org/50012/diff/7/12
File src/virtual-frame-arm.h (right):

http://codereview.chromium.org/50012/diff/7/12#newcode450
Line 450: // Presently does nothing on ARM.
This comment should just be identical to the IA32 one (mutatis mutandis).  "Does
nothing on ARM" has to be maintained (and isn't correct anyway---calling it does
something by terminating the process in release and debug builds).

http://codereview.chromium.org/50012/diff/7/8
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/50012/diff/7/8#newcode501
Line 501: // Before changing an element which is copied, adjust so that the
Change this comment to correspond to the changes to the code.

http://codereview.chromium.org/50012/diff/7/8#newcode506
Line 506: FrameElement VirtualFrame::InvalidateFrameSlot(int index) {
To match the names of everything else, this should be the slightly longer
"InvalidateFrameSlotAt".

http://codereview.chromium.org/50012/diff/7/8#newcode510
Line 510: // Set it to invalid, and free the register.
This comment is confusing.  It mentions freeing a register (the original may not
even be a register), but I don't see any code to do that.

http://codereview.chromium.org/50012/diff/7/8#newcode512
Line 512: elements_[index].clear_copied();
Invalid elements (in fact, all new elements) are already created with their copy
flags cleared, so this line isn't needed.

http://codereview.chromium.org/50012/diff/7/8#newcode523
Line 523: Unuse(original.reg());
I think it's more clear to have the code to set elements_[index] to invalid
here, close to the unuse of the register (even if it means duplicating that line
for the other case).  It's too hard to track related state changes that are
separated too much in the code.

http://codereview.chromium.org/50012/diff/7/8#newcode541
Line 541: FrameElement::SyncField::decode(elements_[i].type()));
Even though VirtualFrame is a friend of FrameElement, it shouldn't depend on the
representation of FrameElements (that it's a bit field).  Go through the
accessors (or add a new one if you need).

http://codereview.chromium.org/50012/diff/7/8#newcode544
Line 544: FrameElement copy = CopyElementAt(i);
This has a slight drawback of marking the new backing store as copied even if
it's not.

http://codereview.chromium.org/50012/diff/7/8#newcode553
Line 553: return copy;
It seems like there's a slicker way to do this.

* Hold off marking the new backing store as copied until the loop looking for
other copies finds one (sometimes we want a copy as the return value, but other
times we will just ignore it anyway).
* For all the copies, can't we just set their data_.index_ member to be the new
backing store (that would automatically preserve the sync flag)?
* Instead of returning a copy of the new backing store or invalid if the
original wasn't copied, can't we just return the index of the new backing store
or kIllegalIndex, and then decide whether to copy it in the caller (where we
know if we want a copy)?

http://codereview.chromium.org/50012/diff/7/8#newcode620
Line 620: elements_[index] = FrameElement::InvalidElement();
If you made InvalidateFrameSlot work on copies and constants, you wouldn't need
this line?

http://codereview.chromium.org/50012/diff/7/8#newcode632
Line 632: // If the stored-to slot may be copied, adjust to preserve the
Adjust this comment to reflect the new code.

http://codereview.chromium.org/50012/diff/7/8#newcode634
Line 634: if (original.is_register() || original.is_memory()) {
It's not obvious why InvalidateFrameSlot won't do the right thing for
non-register, non-memory frame elements.

http://codereview.chromium.org/50012/diff/7/10
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/50012/diff/7/10#newcode436
Line 436: // Helper function to implement the copy-on-write semantics of an
This comment needs to be updated: the main point is that it invalidates a frame
element (not necessarily just before writing to it anymore), unusing registers
and preserving the copy invariants as needed.

http://codereview.chromium.org/50012/diff/7/9
File src/virtual-frame.cc (right):

http://codereview.chromium.org/50012/diff/7/9#newcode380
Line 380: // semantics of copied elements.
Change this comment, it's no longer correct.

http://codereview.chromium.org/50012/diff/7/9#newcode381
Line 381: if (original.is_register() || original.is_memory()) {
It's not obvious why InvalidateFrameSlot won't do the right thing for
non-register, non-memory frame elements, or is it just that it's unnecessary
(the comment should explain).

[William Hesse, 2009-03-23 12:25:23]

Much nicer now.

http://codereview.chromium.org/50012/diff/7/12
File src/virtual-frame-arm.h (right):

http://codereview.chromium.org/50012/diff/7/12#newcode450
Line 450: // Presently does nothing on ARM.
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> This comment should just be identical to the IA32 one (mutatis mutandis). 
"Does
> nothing on ARM" has to be maintained (and isn't correct anyway---calling it
does
> something by terminating the process in release and debug builds).

Done.

http://codereview.chromium.org/50012/diff/7/8
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/50012/diff/7/8#newcode501
Line 501: // Before changing an element which is copied, adjust so that the
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> Change this comment to correspond to the changes to the code.

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode506
Line 506: FrameElement VirtualFrame::InvalidateFrameSlot(int index) {
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> To match the names of everything else, this should be the slightly longer
> "InvalidateFrameSlotAt".

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode510
Line 510: // Set it to invalid, and free the register.
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> This comment is confusing.  It mentions freeing a register (the original may
not
> even be a register), but I don't see any code to do that.

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode512
Line 512: elements_[index].clear_copied();
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> Invalid elements (in fact, all new elements) are already created with their
copy
> flags cleared, so this line isn't needed.

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode523
Line 523: Unuse(original.reg());
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> I think it's more clear to have the code to set elements_[index] to invalid
> here, close to the unuse of the register (even if it means duplicating that
line
> for the other case).  It's too hard to track related state changes that are
> separated too much in the code.

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode541
Line 541: FrameElement::SyncField::decode(elements_[i].type()));
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> Even though VirtualFrame is a friend of FrameElement, it shouldn't depend on
the
> representation of FrameElements (that it's a bit field).  Go through the
> accessors (or add a new one if you need).

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode544
Line 544: FrameElement copy = CopyElementAt(i);
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> This has a slight drawback of marking the new backing store as copied even if
> it's not.

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode553
Line 553: return copy;
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> It seems like there's a slicker way to do this.
> 
> * Hold off marking the new backing store as copied until the loop looking for
> other copies finds one (sometimes we want a copy as the return value, but
other
> times we will just ignore it anyway).
> * For all the copies, can't we just set their data_.index_ member to be the
new
> backing store (that would automatically preserve the sync flag)?
> * Instead of returning a copy of the new backing store or invalid if the
> original wasn't copied, can't we just return the index of the new backing
store
> or kIllegalIndex, and then decide whether to copy it in the caller (where we
> know if we want a copy)?

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode620
Line 620: elements_[index] = FrameElement::InvalidElement();
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> If you made InvalidateFrameSlot work on copies and constants, you wouldn't
need
> this line?

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode632
Line 632: // If the stored-to slot may be copied, adjust to preserve the
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> Adjust this comment to reflect the new code.

Done.

http://codereview.chromium.org/50012/diff/7/8#newcode634
Line 634: if (original.is_register() || original.is_memory()) {
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> It's not obvious why InvalidateFrameSlot won't do the right thing for
> non-register, non-memory frame elements.

Done.

http://codereview.chromium.org/50012/diff/7/10
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/50012/diff/7/10#newcode436
Line 436: // Helper function to implement the copy-on-write semantics of an
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> This comment needs to be updated: the main point is that it invalidates a
frame
> element (not necessarily just before writing to it anymore), unusing registers
> and preserving the copy invariants as needed.

Done.

http://codereview.chromium.org/50012/diff/7/9
File src/virtual-frame.cc (right):

http://codereview.chromium.org/50012/diff/7/9#newcode380
Line 380: // semantics of copied elements.
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> Change this comment, it's no longer correct.

Done.

http://codereview.chromium.org/50012/diff/7/9#newcode381
Line 381: if (original.is_register() || original.is_memory()) {
On 2009/03/20 14:44:56, Kevin Millikin wrote:
> It's not obvious why InvalidateFrameSlot won't do the right thing for
> non-register, non-memory frame elements, or is it just that it's unnecessary
> (the comment should explain).

Done.

[Kevin Millikin (Chromium), 2009-03-23 13:14:09]

LGTM.  Couple formatting issues you should look at.

http://codereview.chromium.org/50012/diff/1007/1008
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/50012/diff/1007/1008#newcode537
Line 537: elements_[new_backing_index] =
FrameElement::RegisterElement(backing_reg,
I still don't like this screwy indentation.  What's wrong with:

if (elements_[new_backing_index].is_synced()) {
  elements_[new_backing_index] =
      FrameElement::RegisterElement(backing_reg, FrameElement::SYNCED);
} else {
  ...
}

If you stick with the ternary operator, please follow the style guide (break
after the opening paren, align each new line containing arguments with a four
space indent, and though not required, don't align the ? and : lines so they
look like arguments (ie, indent them further)).

http://codereview.chromium.org/50012/diff/1007/1008#newcode543
Line 543: for (int j = new_backing_index+1; j < elements_.length(); j++) {
I'd rather see "i" for the index.  Binary operators (+) should be surrounded
with spaces.

http://codereview.chromium.org/50012/diff/1007/1008#newcode566
Line 566: // that register element on the top of the stack.
Be careful in comments about the distinction between "stack" (which exists at
runtime) and "(virtual) frame" which exists at compile time.  Here we're
actually not pushing on the stack, so I'd say something like "allocate to a
register and push on the frame".