Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 3 months ago by Jakob Kummerow Modified:
4 years, 3 months ago Reviewers:
Igor Sheludko, Michael Starzinger CC:
v8-reviews_googlegroups.com, Toon Verwaest Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[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}
Patch Set 1 #
Total comments: 19
Patch Set 2 : rebased #Patch Set 3 : addressed comments #Patch Set 4 : rebased #Patch Set 5 : fix strings of uncacheable indices #
Total comments: 1
Patch Set 6 : reword comment #
Created: 4 years, 3 months ago
Messages
Total messages: 25 (12 generated)
jkummerow@chromium.org changed reviewers: + verwaest@chromium.org
Here is the promised follow-up. PTAL.
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.
==========
jkummerow@chromium.org changed reviewers: + ishell@chromium.org - verwaest@chromium.org
Redirecting to Igor. Please review at your convenience :-)
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.
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.
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.
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.
==========
lgtm
The CQ bit was checked by jkummerow@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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)
mstarzinger@chromium.org changed reviewers: + mstarzinger@chromium.org
LGTM (rubber-stamp on "compiler", didn't look at the rest).
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.
The CQ bit was checked by jkummerow@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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.
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
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}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/71dfcbacc2277d890d914b292f5725a6f3f1a5cf Cr-Commit-Position: refs/heads/master@{#39217} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
