Change FastCharCodeAt to use a virtual frame.

Change FastCharCodeAt to use a virtual frame.

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

[William Hesse, 2009-01-28 14:17:18]

A first look at these changes.  They compile, but there may still be a few
problems.  Particular questions: should we spill object.reg() at the beginning,
since the back edge to try again for cons and sliced strings will spill it?
Should we preallocate ecx, forget it, and allocate it again where needed, rather
than passing it around the loop?

[Kevin Millikin (Chromium), 2009-01-29 10:16:53]

I know this isn't completed or tested, but I found some possible sources of
bugs.

As for your specific questions, I really don't know.  It seems like we should
probably get it working, and then experiment with various versions on benchmarks
where it gets exercised.

http://codereview.chromium.org/19425/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/19425/diff/1/3#newcode3480
Line 3480: set_in_spilled_code(false);
Don't forget to remove this.

http://codereview.chromium.org/19425/diff/1/3#newcode3489
Line 3489: // Load the string into eax and the index into ebx.
Drop this comment.

http://codereview.chromium.org/19425/diff/1/3#newcode3495
Line 3495: Result index = frame_->Pop();  // ebx in previous code
Remove this comment before submitting.

http://codereview.chromium.org/19425/diff/1/3#newcode3497
Line 3497: Result object = frame_->Pop();  // eax in previous code
And this one too.

http://codereview.chromium.org/19425/diff/1/3#newcode3535
Line 3535: object_type.Unuse();
There's no real reason to unuse object_type here.  You could consider just
aliasing it with length, using length where it makes sense, and then unusing
length later.

http://codereview.chromium.org/19425/diff/1/3#newcode3554
Line 3554: // Pass shift_amount register (ecx) to this branch so it can be
merged back
This comment is a little confusing.  shift_amount is called object_type at the
binding of the jump target, and it is live in that block so it was not passed
just to merge it back at the backward edge.

http://codereview.chromium.org/19425/diff/1/3#newcode3571
Line 3571: ascii_string.Bind();
Arguments were passed at ascii_string.Branch, so they should be picked up here
at Bind.

http://codereview.chromium.org/19425/diff/1/3#newcode3574
Line 3574: times_1,
Indentation is off.

http://codereview.chromium.org/19425/diff/1/3#newcode3578
Line 3578: got_char_code.Bind(&object_type);
This is confusing.  The only non-frame value passed from unconditional jump is
object_type, but shift_amount is live after the Bind.

http://codereview.chromium.org/19425/diff/1/3#newcode3585
Line 3585: not_a_flat_string.Bind(&object, &index, &object_type);
A little confusing that there is only one jump to this label, and it passes
shift_reg instead of object_type.

http://codereview.chromium.org/19425/diff/1/3#newcode3588
Line 3588: a_cons_string.Branch(equal, &object, &index, &shift_amount,
not_taken);
You negated the branch but not the hint, for what that's worth.

http://codereview.chromium.org/19425/diff/1/3#newcode3612
Line 3612: frame_->EmitPush(Immediate(Factory::undefined_value()));
This should be a regular Push, not a raw one, since the frame is unspilled.

[William Hesse, 2009-01-30 11:26:19]

http://codereview.chromium.org/19425/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/19425/diff/1/3#newcode3480
Line 3480: set_in_spilled_code(false);
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> Don't forget to remove this.

When I merge with the current version of toiger, I will.  Right now, it is
needed.  Merging with current version of toiger is the next step.

http://codereview.chromium.org/19425/diff/1/3#newcode3489
Line 3489: // Load the string into eax and the index into ebx.
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> Drop this comment.

Done.

http://codereview.chromium.org/19425/diff/1/3#newcode3495
Line 3495: Result index = frame_->Pop();  // ebx in previous code
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> Remove this comment before submitting.

Done.

http://codereview.chromium.org/19425/diff/1/3#newcode3497
Line 3497: Result object = frame_->Pop();  // eax in previous code
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> And this one too.

Done.

http://codereview.chromium.org/19425/diff/1/3#newcode3535
Line 3535: object_type.Unuse();
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> There's no real reason to unuse object_type here.  You could consider just
> aliasing it with length, using length where it makes sense, and then unusing
> length later.

Done.

http://codereview.chromium.org/19425/diff/1/3#newcode3554
Line 3554: // Pass shift_amount register (ecx) to this branch so it can be
merged back
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> This comment is a little confusing.  shift_amount is called object_type at the
> binding of the jump target, and it is live in that block so it was not passed
> just to merge it back at the backward edge.

Done.

http://codereview.chromium.org/19425/diff/1/3#newcode3571
Line 3571: ascii_string.Bind();
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> Arguments were passed at ascii_string.Branch, so they should be picked up here
> at Bind.

Done.

http://codereview.chromium.org/19425/diff/1/3#newcode3574
Line 3574: times_1,
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> Indentation is off.

Done.

http://codereview.chromium.org/19425/diff/1/3#newcode3578
Line 3578: got_char_code.Bind(&object_type);
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> This is confusing.  The only non-frame value passed from unconditional jump is
> object_type, but shift_amount is live after the Bind.

This was an error - shift_amount should not have been used.

Done.

http://codereview.chromium.org/19425/diff/1/3#newcode3585
Line 3585: not_a_flat_string.Bind(&object, &index, &object_type);
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> A little confusing that there is only one jump to this label, and it passes
> shift_reg instead of object_type.

This was an error.

http://codereview.chromium.org/19425/diff/1/3#newcode3588
Line 3588: a_cons_string.Branch(equal, &object, &index, &shift_amount,
not_taken);
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> You negated the branch but not the hint, for what that's worth.

Done.

http://codereview.chromium.org/19425/diff/1/3#newcode3612
Line 3612: frame_->EmitPush(Immediate(Factory::undefined_value()));
On 2009/01/29 10:16:53, Kevin Millikin wrote:
> This should be a regular Push, not a raw one, since the frame is unspilled.

Done.

[Kevin Millikin (Chromium), 2009-01-30 12:44:20]

I didn't pore over the code too closely, but it looks OK.  If it passes all the
tests we have in both modes, then put it in and see if it makes any difference.