Custom call IC-s for String.prototype.{charAt,charCodeAt}.

Custom call IC-s for String.prototype.{charAt,charCodeAt}.

These string methods can be composed from two basic blocks: charCodeAt
and fromCharCode, both of which have fast cases for certain types of
inputs. In this patch these two blocks are refactored to allow
generating the fast cases without having to jump around the slow
cases. In the slow cases since they can now be invoked both from
inline runtime functions and from IC stubs we either have to
save/restore state of the current frame or enter/leave a new internal
frame. This is handled by new RuntimeCallHelper interface. Its 
implementation for virtual frame is based on FrameRegisterState class
extracted from DeferredCode class.

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

[Vitaly Repeshko, 2010-05-17 11:57:45]

Mads,

This patch has to be implemented for all architectures, but I only covered the
ia32 case so that I can use your inputs when porting. We'll need another round
of reviews for the ports.


Thanks,
Vitaly

[Mads Ager (chromium), 2010-05-18 11:00:12]

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

http://codereview.chromium.org/2087009/diff/1/3#newcode6133
src/ia32/codegen-ia32.cc:6133:
deferred->fast_case_generator()->GenerateFast(masm_);
It seems backwards to me that the deferred code object has a generator for the
fast case code. I would rather have something here that generates the fast case
code and uses the deferred code object for bailouts from that. Maybe that means
having static methods for the fast/slow case generation of CharCodeAt that takes
registers and labels as arguments?

http://codereview.chromium.org/2087009/diff/1/3#newcode12645
src/ia32/codegen-ia32.cc:12645: __ Abort("CharCodeAt slow case reached");
I guess the problem here is not that a slow case is reached, but that we fall
through to the slow case where we are expected to jump to one of the labels
instead. Could you make the Abort message reflect that?

http://codereview.chromium.org/2087009/diff/1/4
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/2087009/diff/1/4#newcode922
src/ia32/codegen-ia32.h:922: // Register usage: |object| may be changed to
another string in way
in way -> in a way

[Vitaly Repeshko, 2010-05-19 14:51:22]

Addressed the first round of comments. Also updated ia32 full codegen to support
the new inline runtime functions (ia32 tests pass with --force-full-compiler).
Please take another look.


Thanks,
Vitaly

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

http://codereview.chromium.org/2087009/diff/1/3#newcode6133
src/ia32/codegen-ia32.cc:6133:
deferred->fast_case_generator()->GenerateFast(masm_);
On 2010/05/18 11:00:12, Mads Ager wrote:
> It seems backwards to me that the deferred code object has a generator for the
> fast case code. I would rather have something here that generates the fast
case
> code and uses the deferred code object for bailouts from that. Maybe that
means
> having static methods for the fast/slow case generation of CharCodeAt that
takes
> registers and labels as arguments?

Per our IM discussion because DeferredCode objects need to have access to the
labels, changing ownership without exposing lots of details here is hard.

http://codereview.chromium.org/2087009/diff/1/3#newcode12645
src/ia32/codegen-ia32.cc:12645: __ Abort("CharCodeAt slow case reached");
On 2010/05/18 11:00:12, Mads Ager wrote:
> I guess the problem here is not that a slow case is reached, but that we fall
> through to the slow case where we are expected to jump to one of the labels
> instead. Could you make the Abort message reflect that?

Done.

http://codereview.chromium.org/2087009/diff/1/4
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/2087009/diff/1/4#newcode922
src/ia32/codegen-ia32.h:922: // Register usage: |object| may be changed to
another string in way
On 2010/05/18 11:00:12, Mads Ager wrote:
> in way -> in a way

Done (also below).

[Mads Ager (chromium), 2010-05-20 10:31:30]

LGTM

[Vitaly Repeshko, 2010-05-25 14:03:42]

On 2010/05/20 10:31:30, Mads Ager wrote:
> LGTM

I ported the generators and inline runtime functions to x64. Fixed a bug (found
by sputnik) related in handling of -0 and fractional indices by string[] and
string.charAt. Extended the tests a bit.

After Søren's ARM full-codegen change lands, I'll do a similar port to ARM. The
custom IC-s themselves won't be ported to x64 and ARM in this patch (to minimize
its size).


Thanks,
Vitaly

[Vitaly Repeshko, 2010-05-26 00:38:26]

On 2010/05/25 14:03:42, Vitaly wrote:
> On 2010/05/20 10:31:30, Mads Ager wrote:
> > LGTM
> 
> I ported the generators and inline runtime functions to x64. Fixed a bug
(found
> by sputnik) related in handling of -0 and fractional indices by string[] and
> string.charAt. Extended the tests a bit.
> 
> After Søren's ARM full-codegen change lands, I'll do a similar port to ARM.
The
> custom IC-s themselves won't be ported to x64 and ARM in this patch (to
minimize
> its size).

ARM port is done. I do not plan any further significant changes to this patch.
Please take (hopefully) final look.


Thanks,
Vitaly

[Mads Ager (chromium), 2010-05-26 09:17:07]

LGTM

http://codereview.chromium.org/2087009/diff/16001/17006
File src/codegen.h (right):

http://codereview.chromium.org/2087009/diff/16001/17006#newcode691
src/codegen.h:691: enum StringIndexFlags {
How about renaming these to something like:

INDEX_IS_NUMBER
INDEX_IS_ARRAY_INDEX

I think that makes it easier to read at the call site. 

I would also like the comments on the generators to say how the code generated
depends on these flags.