The check for internalized strings relied on the fact that we had less

The check for internalized strings relied on the fact that we had less
than 64 distinct InstanceTypes. We are hitting that boundary, so this
check needs to be more comprehensive. In fact, two bits need to be
tested: verify that kNotStringTag isn't set, and that kInternalizedTag
is set.

BUG=
R=yangguo@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15358

[mvstanton, 2013-06-26 14:30:32]

Hi Yang, here is the code we discussed, thanks!
--Michael

[Yang, 2013-06-26 15:00:17]

On 2013/06/26 14:30:32, mvstanton wrote:
> Hi Yang, here is the code we discussed, thanks!
> --Michael

LGTM, but I got some suggestions. The comments I left on x64 also apply to other
platforms.

[Yang, 2013-06-26 15:00:27]

comments...

https://codereview.chromium.org/17895002/diff/1009/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/17895002/diff/1009/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:6270: __ cmp(tmp1, Operand(kInternalizedTag |
kStringTag));
See my comment on x64.

https://codereview.chromium.org/17895002/diff/1009/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/17895002/diff/1009/src/x64/code-stubs-x64.cc#...
src/x64/code-stubs-x64.cc:5901: __ j(not_equal, &miss, Label::kNear);
We could be clever here to save one instruction:

SYMBOL_TYPE is right next to the area reserved for internalized strings.

You could check that SYMBOL_TYPE >= tmp1 >= kInternalizedTag.

Add a static assert, something like
STATIC_ASSERT((SYMBOL_TYPE - 1) & kInternalizedMask == kInternalizedTag);
or
STATIC_ASSERT(FIRST_NONSTRING_TYPE == SYMBOL_TYPE)

You could also put this logic into a function called JumpIfNotUniqueName, and
reuse it later too.

https://codereview.chromium.org/17895002/diff/1009/src/x64/code-stubs-x64.cc#...
src/x64/code-stubs-x64.cc:6274: __ j(zero, &maybe_in_dictionary);
JumpIfNotUniqueName that I suggested above would be nice here.

[mvstanton, 2013-06-26 16:09:37]

Thanks Yang, I updated after your comments,
--Michael

https://codereview.chromium.org/17895002/diff/1009/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/17895002/diff/1009/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:6270: __ cmp(tmp1, Operand(kInternalizedTag |
kStringTag));
On 2013/06/26 15:00:27, Yang wrote:
> See my comment on x64.

Done.

https://codereview.chromium.org/17895002/diff/1009/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/17895002/diff/1009/src/x64/code-stubs-x64.cc#...
src/x64/code-stubs-x64.cc:5901: __ j(not_equal, &miss, Label::kNear);
On 2013/06/26 15:00:27, Yang wrote:
> We could be clever here to save one instruction:
> 
> SYMBOL_TYPE is right next to the area reserved for internalized strings.
> 
> You could check that SYMBOL_TYPE >= tmp1 >= kInternalizedTag.
> 
> Add a static assert, something like
> STATIC_ASSERT((SYMBOL_TYPE - 1) & kInternalizedMask == kInternalizedTag);
> or
> STATIC_ASSERT(FIRST_NONSTRING_TYPE == SYMBOL_TYPE)
> 
> You could also put this logic into a function called JumpIfNotUniqueName, and
> reuse it later too.

Done.

https://codereview.chromium.org/17895002/diff/1009/src/x64/code-stubs-x64.cc#...
src/x64/code-stubs-x64.cc:6274: __ j(zero, &maybe_in_dictionary);
On 2013/06/26 15:00:27, Yang wrote:
> JumpIfNotUniqueName that I suggested above would be nice here.

Done.

[Yang, 2013-06-27 07:48:36]

LGTM with one comment.

https://codereview.chromium.org/17895002/diff/6002/src/x64/macro-assembler-x6...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/17895002/diff/6002/src/x64/macro-assembler-x6...
src/x64/macro-assembler-x64.cc:2322: void
MacroAssembler::JumpIfNotUniqueName(Register reg,
This is like, a character-by-character copy. Could you use some template magic
here?

[mvstanton, 2013-06-27 13:35:19]

Thanks Yang, I will submit.

https://codereview.chromium.org/17895002/diff/6002/src/x64/macro-assembler-x6...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/17895002/diff/6002/src/x64/macro-assembler-x6...
src/x64/macro-assembler-x64.cc:2322: void
MacroAssembler::JumpIfNotUniqueName(Register reg,
On 2013/06/27 07:48:36, Yang wrote:
> This is like, a character-by-character copy. Could you use some template magic
> here?

Done.

[mvstanton, 2013-06-27 13:39:56]

Committed patchset #4 manually as r15358 (presubmit successful).