Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(15)

Issue 2358263002: [builtins] migrate C++ String Iterator builtins to baseline TurboFan (Closed)

Created:
3 years, 9 months ago by caitp
Modified:
3 years, 9 months ago
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[builtins] migrate C++ String Iterator builtins to baseline TurboFan Migrate newly added C++ String Iterator builtins to TFJ builtins, per step 4. of the String Iterator Baseline Implementation section of the design doc BUG=v8:5388 R=bmeurer@chromium.org, mstarzinger@chromium.org Committed: https://crrev.com/f9a2c8b1112c4e915df8bc5f7ea1fccdf7a33ff8 Cr-Commit-Position: refs/heads/master@{#39765}

Patch Set 1 #

Patch Set 2 : V2 (fixups?) #

Total comments: 1

Patch Set 3 : v3 (try again, but probably not ._.) #

Total comments: 7

Patch Set 4 : v4 (refactor and try again) #

Patch Set 5 : v5 (CF instead of SF where appropriate) #

Patch Set 6 : v5 + minor cleanup #

Patch Set 7 : v6 (fix toString() behaviour in S.p[@@iterator]) #

Total comments: 11

Patch Set 8 : v7 #

Total comments: 5

Patch Set 9 : some review #

Patch Set 10 : Adapt arguments for TFJ builtins #

Total comments: 14

Patch Set 11 : final nits #

Patch Set 12 : Fix the comment #

Patch Set 13 : add "break;" statement to switch case #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+501 lines, -46 lines) Patch
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +11 lines, -3 lines 0 comments Download
M src/builtins/builtins.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -2 lines 0 comments Download
M src/builtins/builtins-string.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +425 lines, -40 lines 1 comment Download
M src/code-stub-assembler.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +9 lines, -0 lines 0 comments Download
M src/code-stub-assembler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +52 lines, -0 lines 0 comments Download
M src/objects-debug.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 81 (49 generated)
caitp
PTAL I haven't measured if this is really beneficial for the baseline implementation (yet), but ...
3 years, 9 months ago (2016-09-21 21:26:01 UTC) #3
caitp
PTAL I haven't measured if this is really beneficial for the baseline implementation (yet), but ...
3 years, 9 months ago (2016-09-21 21:26:03 UTC) #4
caitp
On 2016/09/21 21:26:03, caitp wrote: > PTAL > > I haven't measured if this is ...
3 years, 9 months ago (2016-09-21 22:11:09 UTC) #7
caitp
https://codereview.chromium.org/2358263002/diff/20001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/20001/src/code-stub-assembler.cc#newcode2498 src/code-stub-assembler.cc:2498: Node* ch = Word32Or(WordShl(lead, Int32Constant(16)), trail); I'm not sure ...
3 years, 9 months ago (2016-09-21 23:07:47 UTC) #12
Benedikt Meurer
Nice. First round of comments. Also please update JSStringIterator::JSStringIteratorVerify() to assert the invariant that the ...
3 years, 9 months ago (2016-09-22 03:48:23 UTC) #17
caitp
https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler.cc#newcode2432 src/code-stub-assembler.cc:2432: Node* CodeStubAssembler::StringFromCodePointAt(Node* string, Node* length, On 2016/09/22 03:48:23, Benedikt ...
3 years, 9 months ago (2016-09-22 13:26:22 UTC) #18
caitp
Now that everything is passing tests, some microbenchmark results: ``` >>> Running suite: JSTests/Strings >>> ...
3 years, 9 months ago (2016-09-22 23:42:27 UTC) #38
caitp
On 2016/09/22 23:42:27, caitp wrote: > Now that everything is passing tests, some microbenchmark results: ...
3 years, 9 months ago (2016-09-23 01:26:36 UTC) #39
Benedikt Meurer
On 2016/09/23 01:26:36, caitp wrote: > On 2016/09/22 23:42:27, caitp wrote: > > Now that ...
3 years, 9 months ago (2016-09-23 03:44:49 UTC) #40
Benedikt Meurer
https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/40001/src/code-stub-assembler.cc#newcode2432 src/code-stub-assembler.cc:2432: Node* CodeStubAssembler::StringFromCodePointAt(Node* string, Node* length, I'd be fine with ...
3 years, 9 months ago (2016-09-23 03:46:24 UTC) #41
caitp
On 2016/09/23 03:44:49, Benedikt Meurer wrote: > On 2016/09/23 01:26:36, caitp wrote: > > On ...
3 years, 9 months ago (2016-09-23 10:14:28 UTC) #42
caitp
On 2016/09/23 10:14:28, caitp wrote: > On 2016/09/23 03:44:49, Benedikt Meurer wrote: > > On ...
3 years, 9 months ago (2016-09-23 12:21:48 UTC) #43
caitp
On 2016/09/23 12:21:48, caitp wrote: > ... > In general, it looks like the TF ...
3 years, 9 months ago (2016-09-23 12:26:57 UTC) #44
caitp
On 2016/09/23 12:26:57, caitp wrote: > On 2016/09/23 12:21:48, caitp wrote: > > ... > ...
3 years, 9 months ago (2016-09-23 14:57:59 UTC) #45
Benedikt Meurer
So for me the most important part is the ability to inline into TurboFan. If ...
3 years, 9 months ago (2016-09-23 17:40:03 UTC) #46
caitp
On 2016/09/23 17:40:03, Benedikt Meurer wrote: > So for me the most important part is ...
3 years, 9 months ago (2016-09-23 18:01:39 UTC) #47
Benedikt Meurer
Thanks for the data. That's very useful to know. One important thing about our (new) ...
3 years, 9 months ago (2016-09-23 18:14:53 UTC) #48
caitp
For some reason rietveld isn't allowing me to edit one of these draft comments, I ...
3 years, 9 months ago (2016-09-23 19:14:16 UTC) #49
Benedikt Meurer
Thanks, getting close. https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-string.cc File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/140001/src/builtins/builtins-string.cc#newcode734 src/builtins/builtins-string.cc:734: static compiler::Node* CreateStringFromCodePointInternal( On 2016/09/23 19:14:16, ...
3 years, 9 months ago (2016-09-26 04:15:40 UTC) #50
caitp
https://codereview.chromium.org/2358263002/diff/160001/src/builtins/builtins-string.cc File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/160001/src/builtins/builtins-string.cc#newcode749 src/builtins/builtins-string.cc:749: assembler->Branch( On 2016/09/26 04:15:40, Benedikt Meurer wrote: > Please ...
3 years, 9 months ago (2016-09-26 14:33:55 UTC) #53
Benedikt Meurer
Final round of comments. LGTM once comments address. Thanks a lot! https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc File src/bootstrapper.cc (right): ...
3 years, 9 months ago (2016-09-26 18:38:22 UTC) #58
caitp
https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc#newcode1450 src/bootstrapper.cc:1450: next->shared()->set_internal_formal_parameter_count(0); On 2016/09/26 18:38:22, Benedikt Meurer wrote: > Comment ...
3 years, 9 months ago (2016-09-27 08:55:57 UTC) #63
Benedikt Meurer
https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc#newcode1450 src/bootstrapper.cc:1450: next->shared()->set_internal_formal_parameter_count(0); On 2016/09/27 08:55:56, caitp wrote: > On 2016/09/26 ...
3 years, 9 months ago (2016-09-27 08:58:54 UTC) #64
caitp
https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/bootstrapper.cc#newcode1450 src/bootstrapper.cc:1450: next->shared()->set_internal_formal_parameter_count(0); On 2016/09/27 08:58:54, Benedikt Meurer wrote: > On ...
3 years, 9 months ago (2016-09-27 09:14:18 UTC) #67
Benedikt Meurer
https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assembler.cc#newcode2463 src/code-stub-assembler.cc:2463: codepoint = Word32Or(WordShl(trail, Int32Constant(16)), lead); I mean the keyword ...
3 years, 9 months ago (2016-09-27 09:16:02 UTC) #70
caitp
https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assembler.cc#newcode2463 src/code-stub-assembler.cc:2463: codepoint = Word32Or(WordShl(trail, Int32Constant(16)), lead); On 2016/09/27 09:16:02, Benedikt ...
3 years, 9 months ago (2016-09-27 09:20:41 UTC) #71
Benedikt Meurer
https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2358263002/diff/200001/src/code-stub-assembler.cc#newcode2463 src/code-stub-assembler.cc:2463: codepoint = Word32Or(WordShl(trail, Int32Constant(16)), lead); Thanks! Now grab a ...
3 years, 9 months ago (2016-09-27 09:52:39 UTC) #72
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2358263002/260001
3 years, 9 months ago (2016-09-27 11:39:26 UTC) #75
commit-bot: I haz the power
Committed patchset #13 (id:260001)
3 years, 9 months ago (2016-09-27 12:04:26 UTC) #76
commit-bot: I haz the power
Patchset 13 (id:??) landed as https://crrev.com/f9a2c8b1112c4e915df8bc5f7ea1fccdf7a33ff8 Cr-Commit-Position: refs/heads/master@{#39765}
3 years, 9 months ago (2016-09-27 12:04:39 UTC) #78
Jakob Kummerow
https://codereview.chromium.org/2358263002/diff/260001/src/builtins/builtins-string.cc File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2358263002/diff/260001/src/builtins/builtins-string.cc#newcode543 src/builtins/builtins-string.cc:543: Node* string_instance_type = assembler->LoadInstanceType(string); This is a bug: instead ...
3 years, 9 months ago (2016-09-29 13:29:49 UTC) #80
Jakob Kummerow
3 years, 9 months ago (2016-09-29 13:30:46 UTC) #81
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:260001) has been created in
https://codereview.chromium.org/2374123005/ by jkummerow@chromium.org.

The reason for reverting is: Introduces an infinite loop (see comment)..

Powered by Google App Engine
This is Rietveld 408576698