[builtins] String.prototype.slice as a CSA builtin.

[builtins] String.prototype.slice as a CSA builtin.

BUG=v8:6370
Review-Url: https://codereview.chromium.org/2870013004
Cr-Commit-Position: refs/heads/master@{#45275}
Committed: https://chromium.googlesource.com/v8/v8/+/0e788e3291c167f1dabf03c9788117224007fc73

[mvstanton, 2017-05-10 08:51:50]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 08:51:57]

Dry run: 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, 2017-05-10 09:27:27]

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

[commit-bot: I haz the power, 2017-05-10 09:27:28]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2017-05-10 13:19:21]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 13:19:25]

Dry run: CQ is trying da patch.

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

[mvstanton, 2017-05-10 13:20:53]

mvstanton@chromium.org changed reviewers:
+ jgruber@chromium.org

[mvstanton, 2017-05-10 13:20:54]

Hi Jakob,
Here is the CL we discussed. I'm not completely sure if the flags field is
desirable on SubString...it saves a *few* instructions, but not as many as I
envisioned. WDYT?
All the best,
--Michael

[commit-bot: I haz the power, 2017-05-10 13:57:34]

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

[commit-bot: I haz the power, 2017-05-10 13:57:35]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-05-11 08:57:41]

Thanks! lgtm with a couple of nits.

Right, we don't save that many instructions unfortunately :/ But I still think
it's nice to have this reduced SubString variant. There's quite a few other
spots where we could save instructions by passing the new flag.

Orthogonal to this CL: I noticed a couple of things we could do to improve code
size:
* Create versions of e.g. ToInteger and ToString that inline the fast path (for
Smis and Strings, respectively) and do a stub call for real conversions.
* Remove the duplicate ToDirectString conversion in SubString (through
StringCharCodeAt), and be smarter about creating the new substring (where we
currently repeat the same code twice).

https://codereview.chromium.org/2870013004/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2870013004/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1814: 2, true);
We could also omit argument adaption for this one (see e.g.
http://crrev.com/2865313002). (Other string builtins should also be updated at
some point).

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
File src/builtins/builtins-string-gen.cc (right):

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:1313: // TODO(jgruber): The old
implementation used Uint32Max instead of SmiMax -
Nit: This comment doesn't apply here.

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:1324: // Conversions and bounds-checks for
{start}.
From a quick look this seems identical to arg handling in substr - could we
reuse that code?

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:1337: Node* const length_plus_start =
SmiAdd(length, start_int);
Nit: Just noticed that this could move into the Select lambda to avoid a useless
addition on the common path.

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:1376: Node* const length_plus_end =
SmiAdd(length, end_int);
Same here.

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

https://codereview.chromium.org/2870013004/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:3561: GotoIfNot(TaggedIsPositiveSmi(from), &runtime);
We should assert the conditions otherwise, same below.

[mvstanton, 2017-05-12 11:00:42]

mvstanton@chromium.org changed reviewers:
+ bmeurer@chromium.org

[mvstanton, 2017-05-12 11:00:43]

Addressed comments, thanks very much!

Benedikt, could you give a rubber-stamp LGTM for the removal of code in
src/js/string.js?

https://codereview.chromium.org/2870013004/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2870013004/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1814: 2, true);
On 2017/05/11 08:57:40, jgruber wrote:
> We could also omit argument adaption for this one (see e.g.
> http://crrev.com/2865313002). (Other string builtins should also be updated at
> some point).

Great idea - I've made the necessary adaptations..

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
File src/builtins/builtins-string-gen.cc (right):

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:1313: // TODO(jgruber): The old
implementation used Uint32Max instead of SmiMax -
On 2017/05/11 08:57:40, jgruber wrote:
> Nit: This comment doesn't apply here.

Done.

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:1324: // Conversions and bounds-checks for
{start}.
On 2017/05/11 08:57:40, jgruber wrote:
> From a quick look this seems identical to arg handling in substr - could we
> reuse that code?

Indeed, I made a utility method in the string code stub assembler.

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:1337: Node* const length_plus_start =
SmiAdd(length, start_int);
On 2017/05/11 08:57:40, jgruber wrote:
> Nit: Just noticed that this could move into the Select lambda to avoid a
useless
> addition on the common path.

Oh yeah! Done.

https://codereview.chromium.org/2870013004/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string-gen.cc:1376: Node* const length_plus_end =
SmiAdd(length, end_int);
On 2017/05/11 08:57:40, jgruber wrote:
> Same here.

Done.

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

https://codereview.chromium.org/2870013004/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:3561: GotoIfNot(TaggedIsPositiveSmi(from), &runtime);
On 2017/05/11 08:57:41, jgruber wrote:
> We should assert the conditions otherwise, same below.

Done.

[mvstanton, 2017-05-12 11:01:58]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 11:02:05]

Dry run: CQ is trying da patch.

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

[Benedikt Meurer, 2017-05-12 11:08:55]

LGTM

[mvstanton, 2017-05-12 11:12:31]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2017-05-12 11:12:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2870013004/#ps80001
(title: "REBASE.")

[commit-bot: I haz the power, 2017-05-12 11:12:35]

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, 2017-05-12 11:35:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494587551898200,
"parent_rev": "752bdcbf0f32d3abb190f3cabc596db9815a294b", "commit_rev":
"0e788e3291c167f1dabf03c9788117224007fc73"}

[commit-bot: I haz the power, 2017-05-12 11:35:40]

Description was changed from

==========
[builtins] String.prototype.slice as a CSA builtin.

BUG=v8:6370
==========

to

==========
[builtins] String.prototype.slice as a CSA builtin.

BUG=v8:6370

Review-Url: https://codereview.chromium.org/2870013004
Cr-Commit-Position: refs/heads/master@{#45275}
Committed:
https://chromium.googlesource.com/v8/v8/+/0e788e3291c167f1dabf03c978811722400...
==========

[commit-bot: I haz the power, 2017-05-12 11:35:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/0e788e3291c167f1dabf03c978811722400...