[regexp] Migrate @@split to TurboFan

[regexp] Migrate @@split to TurboFan

This shows around a 2.2x speedup compared to the old JS implementation (and
3.5x compared to CPP) for the fast path.

Adds ToUint32 to CodeStubAssembler.

BUG=v8:5339
Committed: https://crrev.com/2c3fe6d961e4a496bf5edc33a14c5c09245185c9
Cr-Commit-Position: refs/heads/master@{#41408}

[jgruber, 2016-11-29 08:18:58]

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

[commit-bot: I haz the power, 2016-11-29 08:19:02]

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, 2016-11-29 08:24:36]

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

[commit-bot: I haz the power, 2016-11-29 08:24:36]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[jgruber, 2016-11-29 13:03:11]

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

[commit-bot: I haz the power, 2016-11-29 13:03:22]

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, 2016-11-29 13:06:12]

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

[commit-bot: I haz the power, 2016-11-29 13:06:13]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13375)

[jgruber, 2016-11-29 13:17:32]

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

[commit-bot: I haz the power, 2016-11-29 13:17:42]

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, 2016-11-29 13:43:38]

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

[commit-bot: I haz the power, 2016-11-29 13:43:40]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-11-29 13:51:42]

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

[Igor Sheludko, 2016-11-30 10:56:38]

First round of comments.

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1647: Callable exec_callable =
CodeFactory::RegExpExec(isolate);
You can probably use RegExpExecInternal here since you known that the regexp is
fast.

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1668: a->Bind(&if_didmatch);
I think you can remove this block in favor of {if_limitiszero} defined above to
reduce code size.

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1716: Callable exec_callable =
CodeFactory::RegExpExec(isolate);
Same here.

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1848: Node* const result =
array.ToJSArray(context);
Goto(&out) to generate less code.

[jgruber, 2016-11-30 12:45:11]

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

[commit-bot: I haz the power, 2016-11-30 12:45:19]

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

[jgruber, 2016-11-30 12:45:33]

PTAL

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1647: Callable exec_callable =
CodeFactory::RegExpExec(isolate);
On 2016/11/30 10:56:37, Igor Sheludko wrote:
> You can probably use RegExpExecInternal here since you known that the regexp
is
> fast.

The naming here can be a bit confusing.

* CodeFactory::RegExpExec is our internal RegExpExecStub, which ultimately calls
into Irregexp generated code.
* RegExpPrototypeExecInternal corresponds to RegExp.prototype.exec in the spec
and calls into CF::REE.
* RegExpExec corresponds to RegExpExec in the spec and handles subclassing. It
calls into RegExpPrototypeExecInternal.
* RegExpImpl::Exec is the slow CPP path equivalent to CF::REE.

So depending on the spec, whether we're on the fast/slow path, and whether we're
in CSA or CPP, it can be necessary to call into any of the above functions.

In this case, calling CF::RegExpExec  is actually an optimization and analogous
to previous behavior in the C++ and JS implementations.

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1668: a->Bind(&if_didmatch);
On 2016/11/30 10:56:38, Igor Sheludko wrote:
> I think you can remove this block in favor of {if_limitiszero} defined above
to
> reduce code size.

Done.

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1716: Callable exec_callable =
CodeFactory::RegExpExec(isolate);
On 2016/11/30 10:56:37, Igor Sheludko wrote:
> Same here.

See reply above.

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1848: Node* const result =
array.ToJSArray(context);
On 2016/11/30 10:56:37, Igor Sheludko wrote:
> Goto(&out) to generate less code.

Done.

As an aside, I initially did not Goto(&out) here on purpose to save one jump.

[commit-bot: I haz the power, 2016-11-30 13:17:25]

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

[commit-bot: I haz the power, 2016-11-30 13:17:26]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2016-11-30 20:47:32]

lgtm with nits:

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2532403002/diff/40001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1647: Callable exec_callable =
CodeFactory::RegExpExec(isolate);
On 2016/11/30 12:45:33, jgruber wrote:
> On 2016/11/30 10:56:37, Igor Sheludko wrote:
> > You can probably use RegExpExecInternal here since you known that the regexp
> is
> > fast.
> 
> The naming here can be a bit confusing.
> 
> * CodeFactory::RegExpExec is our internal RegExpExecStub, which ultimately
calls
> into Irregexp generated code.
> * RegExpPrototypeExecInternal corresponds to RegExp.prototype.exec in the spec
> and calls into CF::REE.
> * RegExpExec corresponds to RegExpExec in the spec and handles subclassing. It
> calls into RegExpPrototypeExecInternal.
> * RegExpImpl::Exec is the slow CPP path equivalent to CF::REE.
> 
> So depending on the spec, whether we're on the fast/slow path, and whether
we're
> in CSA or CPP, it can be necessary to call into any of the above functions.
> 
> In this case, calling CF::RegExpExec  is actually an optimization and
analogous
> to previous behavior in the C++ and JS implementations.

Acknowledged.

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1767: CLabel nested_loop(a, vars_count, vars),
nested_loop_out(a);
Consider using CSA::BuildFastLoop(). Maybe in next CL.

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1781: RegExpMatchInfo::kFirstCaptureIndex *
kPointerSize, mode);
I think the generated code will be better if you add kPointerSize to this
constant offset instead of adding one to the index. Please check.

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1886: Node* const uint32_limit =
a.ToUint32(context, maybe_limit);
uint32_limit name is confusing. How about just limit?

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1896: Node* const smi_max =
a.SmiConstant(kSmiMaxValue);
Smi::kMaxValue

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

https://codereview.chromium.org/2532403002/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.cc:3778: Branch(WordIsPositiveSmi(input), &out, &next);
Maybe add a TODO that we can combine this with ToNumber() to get better code. We
just need a ToNumber with multiple exits.

[jgruber, 2016-12-01 07:56:24]

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
File src/builtins/builtins-regexp.cc (right):

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1767: CLabel nested_loop(a, vars_count, vars),
nested_loop_out(a);
On 2016/11/30 20:47:31, Igor Sheludko wrote:
> Consider using CSA::BuildFastLoop(). Maybe in next CL.

Acknowledged.

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1781: RegExpMatchInfo::kFirstCaptureIndex *
kPointerSize, mode);
On 2016/11/30 20:47:31, Igor Sheludko wrote:
> I think the generated code will be better if you add kPointerSize to this
> constant offset instead of adding one to the index. Please check.

You're right, we save one lea instruction. Done.

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1886: Node* const uint32_limit =
a.ToUint32(context, maybe_limit);
On 2016/11/30 20:47:31, Igor Sheludko wrote:
> uint32_limit name is confusing. How about just limit?

Done.

https://codereview.chromium.org/2532403002/diff/60001/src/builtins/builtins-r...
src/builtins/builtins-regexp.cc:1896: Node* const smi_max =
a.SmiConstant(kSmiMaxValue);
On 2016/11/30 20:47:31, Igor Sheludko wrote:
> Smi::kMaxValue

Done.

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

https://codereview.chromium.org/2532403002/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.cc:3778: Branch(WordIsPositiveSmi(input), &out, &next);
On 2016/11/30 20:47:31, Igor Sheludko wrote:
> Maybe add a TODO that we can combine this with ToNumber() to get better code.
We
> just need a ToNumber with multiple exits.

Done.

[jgruber, 2016-12-01 07:56:34]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-12-01 07:56:35]

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/2532403002/#ps100001
(title: "Address comments")

[commit-bot: I haz the power, 2016-12-01 07:56:37]

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-12-01 08:24:57]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480578994770420,
"parent_rev": "2a2dd6ff4920bfb0809550083a21968dc7e551bd", "commit_rev":
"875e51fc3bde79495e3a5ea1b30690564560dd38"}

[commit-bot: I haz the power, 2016-12-01 08:25:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-01 08:25:26]

Description was changed from

==========
[regexp] Migrate @@split to TurboFan

This shows around a 2.2x speedup compared to the old JS implementation (and
3.5x compared to CPP) for the fast path.

Adds ToUint32 to CodeStubAssembler.

BUG=v8:5339
==========

to

==========
[regexp] Migrate @@split to TurboFan

This shows around a 2.2x speedup compared to the old JS implementation (and
3.5x compared to CPP) for the fast path.

Adds ToUint32 to CodeStubAssembler.

BUG=v8:5339

Committed: https://crrev.com/2c3fe6d961e4a496bf5edc33a14c5c09245185c9
Cr-Commit-Position: refs/heads/master@{#41408}
==========

[commit-bot: I haz the power, 2016-12-01 08:25:27]

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