Bypass an expensive computation of a basic block's entry frame for a...

Bypass an expensive computation of a basic block's entry frame for a
common case of a single forward edge (a fall-through).  This includes
exits from deferred code.

The fall-through frame is used, after removing constants and copies
above the high-water mark.

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

[Kevin Millikin (Chromium), 2009-05-14 15:10:10]

This is pretty complicated.  The virtual frame has a bunch of places where we do
almost but not quite the same thing.  We should find a way to abstract them
somehow.

[Kasper Lund, 2009-05-15 06:26:50]

LGTM, but I'd like to get Bill's feedback too.

http://codereview.chromium.org/113400/diff/1/2
File src/ia32/jump-target-ia32.cc (right):

http://codereview.chromium.org/113400/diff/1/2#newcode225
Line 225: 
Too much spacing?

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

http://codereview.chromium.org/113400/diff/1/3#newcode194
Line 194: elements_[i].clear_copied();
Can't you add support for coping the is_copied flag directly? All this clearing,
testing, and setting seems complicated.

[William Hesse, 2009-05-15 07:45:51]

LGTM.

http://codereview.chromium.org/113400/diff/1/2
File src/ia32/jump-target-ia32.cc (right):

http://codereview.chromium.org/113400/diff/1/2#newcode228
Line 228: reaching_frames_.length() == 1) {
Can't this case be reduced to the previous case (1 reaching frame, no linked
frames) using the code below, for both cases, forward and backward links?  Then
the lines below making cgen->frame frames_[0], and dropping frames_[0], could be
before line 198, and reuse that code.  Would that be better?

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

http://codereview.chromium.org/113400/diff/1/3#newcode194
Line 194: elements_[i].clear_copied();
On 2009/05/15 06:26:51, Kasper Lund wrote:
> Can't you add support for coping the is_copied flag directly? All this
clearing,
> testing, and setting seems complicated.

I recall that when we tried any sort of list or count of the number of copies,
so that we could accurately know when the last copy was removed, performance
decreased.

http://codereview.chromium.org/113400/diff/1/3#newcode213
Line 213: Result fresh = cgen_->allocator()->Allocate();
If there is a preference to allocate slots at the top of the frame to registers,
this accomplishes that.
Maybe a comment saying "put copies and constants in registers, working upward to
the top of the frame"

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

http://codereview.chromium.org/113400/diff/1/4#newcode118
Line 118: 
How about:
// Remove copies and constants from the top mergeable_elements of the frame,
// so that a frame of the same height that differs only on
these elements can be merged to it.
// Use JumpTarget::kAllElements if the entire frame should be made mergeable.

[Kevin Millikin (Chromium), 2009-05-15 08:03:34]

http://codereview.chromium.org/113400/diff/1/2
File src/ia32/jump-target-ia32.cc (right):

http://codereview.chromium.org/113400/diff/1/2#newcode225
Line 225: 
On 2009/05/15 06:26:51, Kasper Lund wrote:
> Too much spacing?

Done.

http://codereview.chromium.org/113400/diff/1/2#newcode228
Line 228: reaching_frames_.length() == 1) {
Of course.  I planned on doing that as another change.

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

http://codereview.chromium.org/113400/diff/1/4#newcode118
Line 118: 
On 2009/05/15 07:45:51, William Hesse wrote:
> How about:
> // Remove copies and constants from the top mergeable_elements of the frame,
> // so that a frame of the same height that differs only on
> these elements can be merged to it.
> // Use JumpTarget::kAllElements if the entire frame should be made mergeable.

Seems misleading.  The arbitrary frame could differ anywhere (could have memory
where this one has register, or they could have different registers).  I'd
rather stick to the accurate wording even if it's more verbose.

I'll change the "not allowed" so it's clear it's a postcondition and not a
precondition.

[Kevin Millikin (Chromium), 2009-05-15 08:12:25]

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

http://codereview.chromium.org/113400/diff/1/3#newcode194
Line 194: elements_[i].clear_copied();
It's annoying because we have to see the copy (later) to know if this one should
be marked copied.

The real problem is that the copy flags are conservative (it's safe to be marked
if not actually copied), so there is no good reason for setting or clearing them
at all.  It's here for the merging code but we can refactor that so it doesn't
matter.