Custom call IC for String.fromCharCode.

Custom call IC for String.fromCharCode.

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

[antonm, 2010-09-06 18:26:34]

http://codereview.chromium.org/3291015/diff/1/2
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/3291015/diff/1/2#newcode1532
src/arm/stub-cache-arm.cc:1532: Register result = r0;
looks like result is only used at 1553, maybe don't declare an alias for it as
it confuses me slightly---both receiver and result share the same register

http://codereview.chromium.org/3291015/diff/1/2#newcode1535
src/arm/stub-cache-arm.cc:1535: __ ldr(code, MemOperand(sp, 0 * kPointerSize));
just curious: could it be faster to batch pop registers on fast path and push
them back when going slow case?

http://codereview.chromium.org/3291015/diff/1/2#newcode1540
src/arm/stub-cache-arm.cc:1540: Handle<Map>(string_function->map()),
should we do map check or object identity check?  I am slightly concerned that
you bail out in compile time if objects is not String, but in runtime check is
somewhat different.

And chances are identity check could be faster (we don't share this code between
contexts, do we?)

http://codereview.chromium.org/3291015/diff/1/2#newcode1546
src/arm/stub-cache-arm.cc:1546: STATIC_ASSERT(kStringTag == 0);
typo: kStringTag -> kSmiTag?

http://codereview.chromium.org/3291015/diff/1/2#newcode1551
src/arm/stub-cache-arm.cc:1551: __ and_(code, code,
Operand(Smi::FromInt(0xffff)));
just curious: is it a proper way to convert negative numbers?

http://codereview.chromium.org/3291015/diff/1/4
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3291015/diff/1/4#newcode1735
src/ia32/stub-cache-ia32.cc:1735: __ and_(code, (0xffff << kSmiTagSize) |
kSmiTag);
why not FromInt as in ARM case?

http://codereview.chromium.org/3291015/diff/1/8
File test/mjsunit/string-fromcharcode.js (right):

http://codereview.chromium.org/3291015/diff/1/8#newcode48
test/mjsunit/string-fromcharcode.js:48: assertEquals(expected,
receiver.fromCharCode(0x20 + 0.5));
maybe negative indices cases should be added.

[Mads Ager (chromium), 2010-09-07 06:05:25]

No additional comments. Looks good when Anton is happy. :)

[Vitaly Repeshko, 2010-09-08 14:35:53]

Comments addressed. I extended custom generators to support the call global
case. Please take another look.


Thanks,
Vitaly

http://codereview.chromium.org/3291015/diff/1/2
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/3291015/diff/1/2#newcode1532
src/arm/stub-cache-arm.cc:1532: Register result = r0;
On 2010/09/06 18:26:34, antonm wrote:
> looks like result is only used at 1553, maybe don't declare an alias for it as
> it confuses me slightly---both receiver and result share the same register

This is now rewritten.

http://codereview.chromium.org/3291015/diff/1/2#newcode1535
src/arm/stub-cache-arm.cc:1535: __ ldr(code, MemOperand(sp, 0 * kPointerSize));
On 2010/09/06 18:26:34, antonm wrote:
> just curious: could it be faster to batch pop registers on fast path and push
> them back when going slow case?

Probably yes. But I'm not happy about the extra complexity and deviation from
the intel code.

http://codereview.chromium.org/3291015/diff/1/2#newcode1540
src/arm/stub-cache-arm.cc:1540: Handle<Map>(string_function->map()),
On 2010/09/06 18:26:34, antonm wrote:
> should we do map check or object identity check?  I am slightly concerned that
> you bail out in compile time if objects is not String, but in runtime check is
> somewhat different.
> 
> And chances are identity check could be faster (we don't share this code
between
> contexts, do we?)

As we discussed, identity checks won't work. Anyway this place has been
rewritten as well.

http://codereview.chromium.org/3291015/diff/1/2#newcode1546
src/arm/stub-cache-arm.cc:1546: STATIC_ASSERT(kStringTag == 0);
On 2010/09/06 18:26:34, antonm wrote:
> typo: kStringTag -> kSmiTag?

Oops! Fixed.

http://codereview.chromium.org/3291015/diff/1/2#newcode1551
src/arm/stub-cache-arm.cc:1551: __ and_(code, code,
Operand(Smi::FromInt(0xffff)));
On 2010/09/06 18:26:34, antonm wrote:
> just curious: is it a proper way to convert negative numbers?

I think so. This is covered by the new tests.

http://codereview.chromium.org/3291015/diff/1/4
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3291015/diff/1/4#newcode1735
src/ia32/stub-cache-ia32.cc:1735: __ and_(code, (0xffff << kSmiTagSize) |
kSmiTag);
On 2010/09/06 18:26:34, antonm wrote:
> why not FromInt as in ARM case?

No such and_ in assembler-ia32.

http://codereview.chromium.org/3291015/diff/1/8
File test/mjsunit/string-fromcharcode.js (right):

http://codereview.chromium.org/3291015/diff/1/8#newcode48
test/mjsunit/string-fromcharcode.js:48: assertEquals(expected,
receiver.fromCharCode(0x20 + 0.5));
On 2010/09/06 18:26:34, antonm wrote:
> maybe negative indices cases should be added.

I had one already (0x20 - 0x10000) but now the coverage is extended.

[antonm, 2010-09-08 15:26:05]

LGTM

http://codereview.chromium.org/3291015/diff/10002/11001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/3291015/diff/10002/11001#newcode1224
src/arm/stub-cache-arm.cc:1224: JSObject* holder,
is it true that holder is always a global object?  if yes, could we ensure it
with types or asserts?

http://codereview.chromium.org/3291015/diff/10002/11001#newcode1266
src/arm/stub-cache-arm.cc:1266: __ mov(r3,
Operand(Handle<SharedFunctionInfo>(function->shared())));
maybe __ Move(r3, Handle<...).  But I don't know if it's falling out of favour
or not :)

http://codereview.chromium.org/3291015/diff/10002/11001#newcode1592
src/arm/stub-cache-arm.cc:1592: Register code = r1;
sorry, that's probably only me, but could you rename code to char_code, please:
when I see code I expect v8::internal::Code* around :)

http://codereview.chromium.org/3291015/diff/10002/11001#newcode1610
src/arm/stub-cache-arm.cc:1610: char_from_code_generator.GenerateSlow(masm(),
call_helper);
just to double check: that is necessary for ::GenerateFast at  ln. 1605?  if
yes, could you add a comment here?

http://codereview.chromium.org/3291015/diff/10002/11001#newcode1856
src/arm/stub-cache-arm.cc:1856: GenerateGlobalReceiverCheck(object, holder,
name, &miss);
aha, looks like holder must be a global object, maybe it should go into
GenerateGlobalReceiverCheck signature

http://codereview.chromium.org/3291015/diff/10002/11003
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3291015/diff/10002/11003#newcode1790
src/ia32/stub-cache-ia32.cc:1790: __ and_(code, (0xffff << kSmiTagSize) |
kSmiTag);
could it be rewritten as __ and_(Operand(code), Immediate(Smi::FromInt(0xffff)))
?

http://codereview.chromium.org/3291015/diff/10002/11006
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/3291015/diff/10002/11006#newcode1527
src/x64/stub-cache-x64.cc:1527: // Tail call the full function.
do you want to unify this comment across platforms (see ia32 version and words
about patching)?

[Vitaly Repeshko, 2010-09-09 13:39:54]

On 2010/09/08 15:26:05, antonm wrote:
> LGTM

Thanks! I added a few more asserts, unified the comments, and extended the set
of ia32 and_ signatures to accept smis.

The patch is now submitted.

-- Vitaly