Inline fast cases in string keyed load IC.

Inline fast cases in string keyed load IC.

String keyed load used to call STRING_CHAR_AT builtin that performs
two steps (get a char code, construct a one-char string from the
code), both of which have fast cases implemented as inline runtime
functions. In this chage most of the code from these functions is
extracted to a set of common generator functions in StringStubBase and
the fast cases are grouped together in the IC code.

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

[Vitaly Repeshko, 2010-04-15 23:32:53]

This is ia32-only for now. I'll file a bug to port to other architectures. Once
it's done STRING_CHAR_AT can be removed.

[Søren Thygesen Gjesse, 2010-04-16 07:18:03]

http://codereview.chromium.org/1539039/diff/4001/5003
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/1539039/diff/4001/5003#newcode891
src/ia32/codegen-ia32.h:891: // overhead. Copying of overlapping regions is not
supported.
Drive by comment,

If we will be using these code generation methods from other places than classes
inheriting from StringStubBase then I suggest getting rid of the StringStubBase
class and move the functions to a StringHelper class inside codegen-ia32.cc
(like the FloatingPointHelper class).

[Søren Thygesen Gjesse, 2010-04-16 08:38:33]

LGTM

http://codereview.chromium.org/1539039/diff/4001/5001
File src/globals.h (right):

http://codereview.chromium.org/1539039/diff/4001/5001#newcode222
src/globals.h:222: const int32_t kInt32SignMask = 0x80000000;
type uint32_t instead?

http://codereview.chromium.org/1539039/diff/4001/5002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1539039/diff/4001/5002#newcode12491
src/ia32/codegen-ia32.cc:12491: __ test(index, Immediate(kSmiTagMask |
kInt32SignMask));
kInt32SignMask -> kSmiSignMask?

Also (kSmiTagMask | 0x80000000) is used in quite a few places, please change
these as well (if dealing with checking the sign).

http://codereview.chromium.org/1539039/diff/4001/5002#newcode12533
src/ia32/codegen-ia32.cc:12533: // Check that the right hand side is the empty
string (ie if this is really a
Check that -> Check if/Check whether?

http://codereview.chromium.org/1539039/diff/4001/5002#newcode12542
src/ia32/codegen-ia32.cc:12542: __ movzx_b(result, FieldOperand(result,
Map::kInstanceTypeOffset));
In this case we know that the string is either a flat or an external string. I
am not sure whether it makes sense to use that information.

http://codereview.chromium.org/1539039/diff/4001/5003
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/1539039/diff/4001/5003#newcode931
src/ia32/codegen-ia32.h:931: // Otherwise, scratch and result clobbered.
result clobbered -> result are clobbered

[Vitaly Repeshko, 2010-04-17 00:42:15]

Søren, thanks a lot for the review! All issues addressed. Please take another
look.


-- Vitaly

http://codereview.chromium.org/1539039/diff/4001/5001
File src/globals.h (right):

http://codereview.chromium.org/1539039/diff/4001/5001#newcode222
src/globals.h:222: const int32_t kInt32SignMask = 0x80000000;
On 2010/04/16 08:38:33, Søren Gjesse wrote:
> type uint32_t instead?

Our similar constants in include/v8.h are of type intptr_t and it turned out we
already have the sign mask constant for intptr_t. Since this mask happens to
also work for smis I reused it in the definition of kSmiSignMask.

http://codereview.chromium.org/1539039/diff/4001/5002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1539039/diff/4001/5002#newcode12491
src/ia32/codegen-ia32.cc:12491: __ test(index, Immediate(kSmiTagMask |
kInt32SignMask));
On 2010/04/16 08:38:33, Søren Gjesse wrote:
> kInt32SignMask -> kSmiSignMask?
> 
> Also (kSmiTagMask | 0x80000000) is used in quite a few places, please change
> these as well (if dealing with checking the sign).

Done. Changed all ia32 places I could find where I was sure the sign is being
tested.

http://codereview.chromium.org/1539039/diff/4001/5002#newcode12533
src/ia32/codegen-ia32.cc:12533: // Check that the right hand side is the empty
string (ie if this is really a
On 2010/04/16 08:38:33, Søren Gjesse wrote:
> Check that -> Check if/Check whether?

Done.

http://codereview.chromium.org/1539039/diff/4001/5002#newcode12542
src/ia32/codegen-ia32.cc:12542: __ movzx_b(result, FieldOperand(result,
Map::kInstanceTypeOffset));
On 2010/04/16 08:38:33, Søren Gjesse wrote:
> In this case we know that the string is either a flat or an external string. I
> am not sure whether it makes sense to use that information.

I'm not sure I follow you here. Do you mean we know this about the first
component of the cons string? I think we can still have e.g. a cons string
there.

http://codereview.chromium.org/1539039/diff/4001/5003
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/1539039/diff/4001/5003#newcode891
src/ia32/codegen-ia32.h:891: // overhead. Copying of overlapping regions is not
supported.
On 2010/04/16 07:18:03, Søren Gjesse wrote:
> Drive by comment,
> 
> If we will be using these code generation methods from other places than
classes
> inheriting from StringStubBase then I suggest getting rid of the
StringStubBase
> class and move the functions to a StringHelper class inside codegen-ia32.cc
> (like the FloatingPointHelper class).

Done. StringHelper is now a simple namespace with helper functions. We still
need it in the header file to allow calls from ic-ia32.cc.

http://codereview.chromium.org/1539039/diff/4001/5003#newcode931
src/ia32/codegen-ia32.h:931: // Otherwise, scratch and result clobbered.
On 2010/04/16 08:38:33, Søren Gjesse wrote:
> result clobbered -> result are clobbered

Done.

[Søren Thygesen Gjesse, 2010-04-19 06:38:45]

LGTM

http://codereview.chromium.org/1539039/diff/4001/5002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1539039/diff/4001/5002#newcode12542
src/ia32/codegen-ia32.cc:12542: __ movzx_b(result, FieldOperand(result,
Map::kInstanceTypeOffset));
On 2010/04/17 00:42:15, Vitaly wrote:
> On 2010/04/16 08:38:33, Søren Gjesse wrote:
> > In this case we know that the string is either a flat or an external string.
I
> > am not sure whether it makes sense to use that information.
> 
> I'm not sure I follow you here. Do you mean we know this about the first
> component of the cons string? I think we can still have e.g. a cons string
> there.
> 

When the second string is the empty string the first is known to be a flat
string. It might still be external though.

http://codereview.chromium.org/1539039/diff/12001/13003
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/1539039/diff/12001/13003#newcode886
src/ia32/codegen-ia32.h:886: class StringHelper : public AllStatic {
Please move the declaration of this class to codegen-ia32.cc, as it is not used
outside.