Inline part of RawSyncElementAt, improve PrepareForCall

Inline part of RawSyncElementAt, split the rest into two
functions.  Improve PrepareForCall, SyncRange, and SyncElementAt.


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

[Kevin Millikin (Chromium), 2009-03-25 13:40:00]

This looks good.

You might consider manually inlining RawSyncElementAt at its two call sites and
getting rid of its definition.  Then, at the call site in SyncRange (in the
loop), you can probably simplify further to just call the right one of the
lower-level sync functions.

If you do that, I'd change the names RawSyncElementBelowStackPointer =>
SyncElementBelowStackPointer and RawSyncElementByPushing =>
SyncElementByPushing.

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

http://codereview.chromium.org/49029/diff/1/2#newcode67
Line 67: break;  // The current value on the stack is some safe value.
I'd get rid of the comment.  It's not clear what "safe" means (it's not safe for
space usage, for instance) and this is not the place to explain it anyway.

http://codereview.chromium.org/49029/diff/1/2#newcode70
Line 70: // There was an early bailout for synced elements
Not an early bailout, it's a precondition (and you might consider to ASSERT that
!element.is_synced() to explicitly catch violations).

http://codereview.chromium.org/49029/diff/1/2#newcode123
Line 123: // There was an early bailout for invalid and synced elements
Not an early bailout, but a precondition.  In this case, it's impossible to have
memory elements (or any synced elements) above the stack pointer.

[William Hesse, 2009-03-25 15:23:02]

Inlined RawSyncElementAt at call sites, optimized by hand.

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

http://codereview.chromium.org/49029/diff/1/2#newcode67
Line 67: break;  // The current value on the stack is some safe value.
On 2009/03/25 13:40:00, Kevin Millikin wrote:
> I'd get rid of the comment.  It's not clear what "safe" means (it's not safe
for
> space usage, for instance) and this is not the place to explain it anyway.

Done.

http://codereview.chromium.org/49029/diff/1/2#newcode70
Line 70: // There was an early bailout for synced elements
On 2009/03/25 13:40:00, Kevin Millikin wrote:
> Not an early bailout, it's a precondition (and you might consider to ASSERT
that
> !element.is_synced() to explicitly catch violations).

Done.

http://codereview.chromium.org/49029/diff/1/2#newcode123
Line 123: // There was an early bailout for invalid and synced elements
On 2009/03/25 13:40:00, Kevin Millikin wrote:
> Not an early bailout, but a precondition.  In this case, it's impossible to
have
> memory elements (or any synced elements) above the stack pointer.

Done.

[Kevin Millikin (Chromium), 2009-03-25 15:58:57]

Oh, and LGTM.

http://codereview.chromium.org/49029/diff/2007/2010
File src/virtual-frame.cc (right):

http://codereview.chromium.org/49029/diff/2007/2010#newcode216
Line 216: // [min(stack_pointer_,begin), end).
You mean min(stack_pointer + 1, begin)?

It might be better in the sense of avoiding surprise to not make this general
and to require the caller to give a proper range (ie, beginning at or below
stack_pointer + 1).  It would be more explicit at the calling sites (both of
them!) what's going on (and I think it's true now anyway).

http://codereview.chromium.org/49029/diff/2007/2010#newcode220
Line 220: if (begin > stack_pointer_) {
This whole think is just complicated enough to need some comments so it doesn't
get off by one when maintaining it.

[William Hesse, 2009-04-01 16:11:08]

http://codereview.chromium.org/49029/diff/2007/2010
File src/virtual-frame.cc (right):

http://codereview.chromium.org/49029/diff/2007/2010#newcode216
Line 216: // [min(stack_pointer_,begin), end).
On 2009/03/25 15:58:57, Kevin Millikin wrote:
> You mean min(stack_pointer + 1, begin)?
> 
> It might be better in the sense of avoiding surprise to not make this general
> and to require the caller to give a proper range (ie, beginning at or below
> stack_pointer + 1).  It would be more explicit at the calling sites (both of
> them!) what's going on (and I think it's true now anyway).

The function has been changed differently, and commented.
It does seem like the correct place to make sure that begin
is less than or equal to stack_pointer_ + 1, rather than at each call site.