Experimental: for forward CFG edges, generate the code to merge to an...

Experimental: for forward CFG edges, generate the code to merge to an
expected frame just before the entry to the basic block, rather than
at each exit from a preceding block.

This will enable us to make better decisions about the frame layout
for the block (better than first-come-first-served), to eliminate
silly jumps caused by empty merge code, and to eliminate the
constraint that the frame has to be mergable in blocks (that are only
targeted by forward CFG edges).

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

[Kasper Lund, 2009-01-15 10:15:02]

LGTM. I would be great to run the new compiler through Valgrind at some point to
make sure that the virtual frame allocation/deallocation is tight.

http://codereview.chromium.org/18089/diff/1/7
File src/jump-target.h (right):

http://codereview.chromium.org/18089/diff/1/7#newcode38
Line 38: // TODO(): Update this comment.
You should probably just do this.

http://codereview.chromium.org/18089/diff/1/7#newcode102
Line 102: expected_frame_ = NULL;
Do we need to delete the expected frame here before setting it to NULL? If not,
it should probably be mentioned in a comment.

http://codereview.chromium.org/18089/diff/1/7#newcode160
Line 160: void AddReachingFrame(VirtualFrame* frame) {
I don't know if this really should be inline. Maybe it would be better to move
it to the .cc file?

[William Hesse, 2009-01-15 12:21:54]

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

http://codereview.chromium.org/18089/diff/1/5#newcode106
Line 106: ASSERT(!cgen_->has_cc());
This looks like a duplicate line.

http://codereview.chromium.org/18089/diff/1/7
File src/jump-target.h (right):

http://codereview.chromium.org/18089/diff/1/7#newcode127
Line 127: // become the current frame after the bind.
Are we allowing non-mergable expected frames, which might be changed to be
mergable or merged when bound, so that the frame after the binding site won't be
exactly the expected frame?

http://codereview.chromium.org/18089/diff/1/7#newcode184
Line 184: // jump target shadows other one, which is hidden as the
Not grammatical, confusing.

http://codereview.chromium.org/18089/diff/1/7#newcode199
Line 199: JumpTarget* other_target() const { return other_target_; }
Could this be called "active" target?  This is the JumpTarget passed to the
constructor, which always seems to be the active one, or the one that is "seen".
 The ShadowTarget represents the hidden target while it is masked by another,
and the target that was used to hide the original, after the hiding is over,
right?

http://codereview.chromium.org/18089/diff/1/4
File src/virtual-frame-ia32.cc (left):

http://codereview.chromium.org/18089/diff/1/4#oldcode321
Line 321: cgen_->SetFrame(this, &non_frame_registers);
Can you put an assert in, that the frame is its cgen's current frame?

[William Hesse, 2009-01-15 12:22:41]

Also, LGTM.
On 2009/01/15 12:21:54, William Hesse wrote:
> http://codereview.chromium.org/18089/diff/1/5
> File src/jump-target-ia32.cc (right):
> 
> http://codereview.chromium.org/18089/diff/1/5#newcode106
> Line 106: ASSERT(!cgen_->has_cc());
> This looks like a duplicate line.
> 
> http://codereview.chromium.org/18089/diff/1/7
> File src/jump-target.h (right):
> 
> http://codereview.chromium.org/18089/diff/1/7#newcode127
> Line 127: // become the current frame after the bind.
> Are we allowing non-mergable expected frames, which might be changed to be
> mergable or merged when bound, so that the frame after the binding site won't
be
> exactly the expected frame?
> 
> http://codereview.chromium.org/18089/diff/1/7#newcode184
> Line 184: // jump target shadows other one, which is hidden as the
> Not grammatical, confusing.
> 
> http://codereview.chromium.org/18089/diff/1/7#newcode199
> Line 199: JumpTarget* other_target() const { return other_target_; }
> Could this be called "active" target?  This is the JumpTarget passed to the
> constructor, which always seems to be the active one, or the one that is
"seen".
>  The ShadowTarget represents the hidden target while it is masked by another,
> and the target that was used to hide the original, after the hiding is over,
> right?
> 
> http://codereview.chromium.org/18089/diff/1/4
> File src/virtual-frame-ia32.cc (left):
> 
> http://codereview.chromium.org/18089/diff/1/4#oldcode321
> Line 321: cgen_->SetFrame(this, &non_frame_registers);
> Can you put an assert in, that the frame is its cgen's current frame?

[Kevin Millikin (Chromium), 2009-01-15 13:08:18]

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

http://codereview.chromium.org/18089/diff/1/5#newcode106
Line 106: ASSERT(!cgen_->has_cc());
On 2009/01/15 12:21:54, William Hesse wrote:
> This looks like a duplicate line.

Oops.  Fixed.

http://codereview.chromium.org/18089/diff/1/7
File src/jump-target.h (right):

http://codereview.chromium.org/18089/diff/1/7#newcode38
Line 38: // TODO(): Update this comment.
On 2009/01/15 10:15:02, Kasper Lund wrote:
> You should probably just do this.

Done.

http://codereview.chromium.org/18089/diff/1/7#newcode102
Line 102: expected_frame_ = NULL;
On 2009/01/15 10:15:02, Kasper Lund wrote:
> Do we need to delete the expected frame here before setting it to NULL? If
not,
> it should probably be mentioned in a comment.

We don't want to delete it because it has normally been copied.  I think the
comment here before the function is pretty clear.

http://codereview.chromium.org/18089/diff/1/7#newcode127
Line 127: // become the current frame after the bind.
On 2009/01/15 12:21:54, William Hesse wrote:
> Are we allowing non-mergable expected frames, which might be changed to be
> mergable or merged when bound, so that the frame after the binding site won't
be
> exactly the expected frame?

The frame after the binding site will always be the expected one.  I've just
changed the comment to be more specific.

http://codereview.chromium.org/18089/diff/1/7#newcode160
Line 160: void AddReachingFrame(VirtualFrame* frame) {
On 2009/01/15 10:15:02, Kasper Lund wrote:
> I don't know if this really should be inline. Maybe it would be better to move
> it to the .cc file?

Done.

http://codereview.chromium.org/18089/diff/1/7#newcode184
Line 184: // jump target shadows other one, which is hidden as the
On 2009/01/15 12:21:54, William Hesse wrote:
> Not grammatical, confusing.

Added "the".

http://codereview.chromium.org/18089/diff/1/7#newcode199
Line 199: JumpTarget* other_target() const { return other_target_; }
On 2009/01/15 12:21:54, William Hesse wrote:
> Could this be called "active" target?  This is the JumpTarget passed to the
> constructor, which always seems to be the active one, or the one that is
"seen".
>  The ShadowTarget represents the hidden target while it is masked by another,
> and the target that was used to hide the original, after the hiding is over,
> right?

No.  After shadowing they are both active.  I can't think of any better name for
this thing that is accurate for its whole life.  It is simply the "other"
target, the one that is not represented by the shadow target.

http://codereview.chromium.org/18089/diff/1/4
File src/virtual-frame-ia32.cc (left):

http://codereview.chromium.org/18089/diff/1/4#oldcode321
Line 321: cgen_->SetFrame(this, &non_frame_registers);
On 2009/01/15 12:21:54, William Hesse wrote:
> Can you put an assert in, that the frame is its cgen's current frame?

Done.