[stubs] Consolidate TryToName implementation

[stubs] Consolidate TryToName implementation

This extends TryToName by HeapNumber-to-intptr support and cached array
index retrieval from non-internalized strings, and uses it in the
KeyedLoadIC_Generic stub.

Bonus: avoid needless movsxlq on x64 in LoadFixed{,Double}ArrayElement
helpers by introducing INTPTR_PARAMETER mode.

Committed: https://crrev.com/71dfcbacc2277d890d914b292f5725a6f3f1a5cf
Cr-Commit-Position: refs/heads/master@{#39217}

[Jakob Kummerow, 2016-08-25 16:59:25]

jkummerow@chromium.org changed reviewers:
+ verwaest@chromium.org

[Jakob Kummerow, 2016-08-25 16:59:26]

Here is the promised follow-up. PTAL.

[Jakob Kummerow, 2016-08-26 16:12:45]

Description was changed from

==========
[TF stubs] Consolidate TryToName implementation

This extends TryToName by HeapNumber-to-intptr support and cached array
index retrieval from non-internalized stringsn, and uses it in the
KeyedLoadIC_Generic stub.

Bonus: avoid needless movsxlq on x64 in LoadFixed{,Double}ArrayElement
helpers by introducing INTPTR_PARAMETER mode.
==========

to

==========
[TF stubs] Consolidate TryToName implementation

This extends TryToName by HeapNumber-to-intptr support and cached array
index retrieval from non-internalized strings, and uses it in the
KeyedLoadIC_Generic stub.

Bonus: avoid needless movsxlq on x64 in LoadFixed{,Double}ArrayElement
helpers by introducing INTPTR_PARAMETER mode.
==========

[Jakob Kummerow, 2016-09-01 08:44:33]

jkummerow@chromium.org changed reviewers:
+ ishell@chromium.org
- verwaest@chromium.org

[Jakob Kummerow, 2016-09-01 08:44:34]

Redirecting to Igor. Please review at your convenience :-)

[Igor Sheludko, 2016-09-01 14:31:31]

First round of comments.

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2274: Int32GreaterThan(key_instance_type,
Int32Constant(LAST_UNIQUE_NAME_TYPE)),
It looks like we are missing FIRST_STRING_TYPE and LAST_STRING_TYPE pseudo-types
here. How about adding them and
  STATIC_ASSERT(FIRST_STRING_TYPE == FIRST_TYPE);
or at least doing something like:
  STATIC_ASSERT(FIRST_NAME_TYPE == FIRST_TYPE);
  <if> (key_instance_type >= FIRST_NONSTRING_TYPE) {
    ...
  }

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2280: GotoIf(Word32Equal(contains_index,
Int32Constant(0)), &if_hascachedindex);
What if the hash field is not computed yet? We can avoid this check only for
internalized strings.

You can and the hash field with (Name::kContainsCachedArrayIndexMask |
Name::kHashNotComputedMask) and goto if_hascachedindex if the result is equal to
Name::kHashNotComputedMask.

Please add respective tests.

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2382: void
CodeStubAssembler::NumberDictionaryLookup(Node* dictionary, Node* key,
Please rename |key| -> |intptr_index| for readability.

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2810: Node* instance_type, Node* index,
Please rename |index| -> |intptr_index| for readability.

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:3507: // |key| should be untagged (int32).
How about renaming |key| to |intptr_index| for readability? In my understanding
key is something tagged.

https://codereview.chromium.org/2277363002/diff/1/src/compiler/code-assembler.h
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2277363002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:286: Node* RoundIntPtrToFloat64(Node* value);
Comment? Just to follow the code style of this file.

https://codereview.chromium.org/2277363002/diff/1/test/cctest/test-code-stub-...
File test/cctest/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/2277363002/diff/1/test/cctest/test-code-stub-...
test/cctest/test-code-stub-assembler.cc:220: key->AsArrayIndex(&dummy);
CHECK(...)?

https://codereview.chromium.org/2277363002/diff/1/test/cctest/test-code-stub-...
test/cctest/test-code-stub-assembler.cc:221: DCHECK(key->HasHashCode());
DCHECK -> CHECK

https://codereview.chromium.org/2277363002/diff/1/test/cctest/test-code-stub-...
test/cctest/test-code-stub-assembler.cc:222:
DCHECK(!key->IsInternalizedString());
Same here.

[Igor Sheludko, 2016-09-01 15:52:17]

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2280: GotoIf(Word32Equal(contains_index,
Int32Constant(0)), &if_hascachedindex);
On 2016/09/01 14:31:30, Igor Sheludko wrote:
> What if the hash field is not computed yet? We can avoid this check only for
> internalized strings.
> 
> You can and the hash field with (Name::kContainsCachedArrayIndexMask |
> Name::kHashNotComputedMask) and goto if_hascachedindex if the result is equal
to
> Name::kHashNotComputedMask.
> 
> Please add respective tests.

Ignore this comment. We set the hash field of a newly created string to
Name::kEmptyHashField constant which has the Name::kIsNotArrayIndexMask bit set
so the "if_hascachedindex" path will not be taken.

[Jakob Kummerow, 2016-09-05 13:15:54]

PTAL.

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2274: Int32GreaterThan(key_instance_type,
Int32Constant(LAST_UNIQUE_NAME_TYPE)),
On 2016/09/01 14:31:30, Igor Sheludko wrote:
> It looks like we are missing FIRST_STRING_TYPE and LAST_STRING_TYPE
pseudo-types
> here. How about adding them and
>   STATIC_ASSERT(FIRST_STRING_TYPE == FIRST_TYPE);
> or at least doing something like:
>   STATIC_ASSERT(FIRST_NAME_TYPE == FIRST_TYPE);
>   <if> (key_instance_type >= FIRST_NONSTRING_TYPE) {
>     ...
>   }

Done.

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2280: GotoIf(Word32Equal(contains_index,
Int32Constant(0)), &if_hascachedindex);
On 2016/09/01 15:52:17, Igor Sheludko wrote:
> On 2016/09/01 14:31:30, Igor Sheludko wrote:
> > What if the hash field is not computed yet? We can avoid this check only for
> > internalized strings.
> > 
> > You can and the hash field with (Name::kContainsCachedArrayIndexMask |
> > Name::kHashNotComputedMask) and goto if_hascachedindex if the result is
equal
> to
> > Name::kHashNotComputedMask.
> > 
> > Please add respective tests.
> 
> Ignore this comment. We set the hash field of a newly created string to
> Name::kEmptyHashField constant which has the Name::kIsNotArrayIndexMask bit
set
> so the "if_hascachedindex" path will not be taken.

Acknowledged. It's also what the handwritten KeyedLoadIC_Generic has always been
doing. I've added a test nevertheless.

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2382: void
CodeStubAssembler::NumberDictionaryLookup(Node* dictionary, Node* key,
On 2016/09/01 14:31:30, Igor Sheludko wrote:
> Please rename |key| -> |intptr_index| for readability.

Done.

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2810: Node* instance_type, Node* index,
On 2016/09/01 14:31:30, Igor Sheludko wrote:
> Please rename |index| -> |intptr_index| for readability.

Done.

https://codereview.chromium.org/2277363002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:3507: // |key| should be untagged (int32).
On 2016/09/01 14:31:30, Igor Sheludko wrote:
> How about renaming |key| to |intptr_index| for readability? In my
understanding
> key is something tagged.

Done.

https://codereview.chromium.org/2277363002/diff/1/src/compiler/code-assembler.h
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2277363002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:286: Node* RoundIntPtrToFloat64(Node* value);
On 2016/09/01 14:31:31, Igor Sheludko wrote:
> Comment? Just to follow the code style of this file.

Done.

https://codereview.chromium.org/2277363002/diff/1/test/cctest/test-code-stub-...
File test/cctest/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/2277363002/diff/1/test/cctest/test-code-stub-...
test/cctest/test-code-stub-assembler.cc:220: key->AsArrayIndex(&dummy);
On 2016/09/01 14:31:31, Igor Sheludko wrote:
> CHECK(...)?

Done.

https://codereview.chromium.org/2277363002/diff/1/test/cctest/test-code-stub-...
test/cctest/test-code-stub-assembler.cc:221: DCHECK(key->HasHashCode());
On 2016/09/01 14:31:31, Igor Sheludko wrote:
> DCHECK -> CHECK

Done. (I don't see the necessity, as that's not the point of the test, it just
verifies that the test tests what it intends to test, which IMHO is sufficient
to verify in Debug mode, but fine, whatever :-) .)

https://codereview.chromium.org/2277363002/diff/1/test/cctest/test-code-stub-...
test/cctest/test-code-stub-assembler.cc:222:
DCHECK(!key->IsInternalizedString());
On 2016/09/01 14:31:31, Igor Sheludko wrote:
> Same here.

Done.

[Jakob Kummerow, 2016-09-05 13:16:35]

Description was changed from

==========
[TF stubs] Consolidate TryToName implementation

This extends TryToName by HeapNumber-to-intptr support and cached array
index retrieval from non-internalized strings, and uses it in the
KeyedLoadIC_Generic stub.

Bonus: avoid needless movsxlq on x64 in LoadFixed{,Double}ArrayElement
helpers by introducing INTPTR_PARAMETER mode.
==========

to

==========
[stubs] Consolidate TryToName implementation

This extends TryToName by HeapNumber-to-intptr support and cached array
index retrieval from non-internalized strings, and uses it in the
KeyedLoadIC_Generic stub.

Bonus: avoid needless movsxlq on x64 in LoadFixed{,Double}ArrayElement
helpers by introducing INTPTR_PARAMETER mode.
==========

[Igor Sheludko, 2016-09-05 16:08:59]

lgtm

[Jakob Kummerow, 2016-09-06 13:57:26]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-09-06 13:57:27]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2277363002/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2016-09-06 13:57:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 14:01:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 14:01:36]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23415)

[Michael Starzinger, 2016-09-06 14:04:32]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2016-09-06 14:04:32]

LGTM (rubber-stamp on "compiler", didn't look at the rest).

[Igor Sheludko, 2016-09-06 15:35:27]

lgtm with a nit:

https://codereview.chromium.org/2277363002/diff/80001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2277363002/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:2283: // No cached array index. Maybe it's an
uncacheable index?
How about saying like this to be more clear:
// No cached array index. Check if it is a string containing index which
// can't be encoded in the hash_field. Handle this case in runtime.

[Jakob Kummerow, 2016-09-06 15:46:38]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-09-06 15:46:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2277363002/#ps100001
(title: "reword comment")

[commit-bot: I haz the power, 2016-09-06 15:46:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 16:17:12]

Description was changed from

==========
[stubs] Consolidate TryToName implementation

This extends TryToName by HeapNumber-to-intptr support and cached array
index retrieval from non-internalized strings, and uses it in the
KeyedLoadIC_Generic stub.

Bonus: avoid needless movsxlq on x64 in LoadFixed{,Double}ArrayElement
helpers by introducing INTPTR_PARAMETER mode.
==========

to

==========
[stubs] Consolidate TryToName implementation

This extends TryToName by HeapNumber-to-intptr support and cached array
index retrieval from non-internalized strings, and uses it in the
KeyedLoadIC_Generic stub.

Bonus: avoid needless movsxlq on x64 in LoadFixed{,Double}ArrayElement
helpers by introducing INTPTR_PARAMETER mode.
==========

[commit-bot: I haz the power, 2016-09-06 16:17:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-06 16:18:20]

Description was changed from

==========
[stubs] Consolidate TryToName implementation

This extends TryToName by HeapNumber-to-intptr support and cached array
index retrieval from non-internalized strings, and uses it in the
KeyedLoadIC_Generic stub.

Bonus: avoid needless movsxlq on x64 in LoadFixed{,Double}ArrayElement
helpers by introducing INTPTR_PARAMETER mode.
==========

to

==========
[stubs] Consolidate TryToName implementation

This extends TryToName by HeapNumber-to-intptr support and cached array
index retrieval from non-internalized strings, and uses it in the
KeyedLoadIC_Generic stub.

Bonus: avoid needless movsxlq on x64 in LoadFixed{,Double}ArrayElement
helpers by introducing INTPTR_PARAMETER mode.

Committed: https://crrev.com/71dfcbacc2277d890d914b292f5725a6f3f1a5cf
Cr-Commit-Position: refs/heads/master@{#39217}
==========

[commit-bot: I haz the power, 2016-09-06 16:18:21]

Patchset 6 (id:??) landed as
https://crrev.com/71dfcbacc2277d890d914b292f5725a6f3f1a5cf
Cr-Commit-Position: refs/heads/master@{#39217}