[builtins] Add String.prototype.indexOf fast path in TF

[builtins] Add String.prototype.indexOf fast path in TF

Review-Url: https://codereview.chromium.org/2638393002
Cr-Commit-Position: refs/heads/master@{#42695}
Committed: https://chromium.googlesource.com/v8/v8/+/439a8edcf77947777c5c757b0dec0363d9446093

[Camillo Bruni, 2017-01-18 13:48:50]

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

[commit-bot: I haz the power, 2017-01-18 13:48:54]

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-01-18 13:51:44]

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

[commit-bot: I haz the power, 2017-01-18 13:51:45]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15869)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/32703)

[Camillo Bruni, 2017-01-18 14:00:24]

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

[Camillo Bruni, 2017-01-18 14:00:25]

PTAL minus merge conflicts due to list sorting...

[Benedikt Meurer, 2017-01-18 18:10:41]

I'd prefer to add only the indexOf change, and do all the cleanups in separate
CLs. That's easier to deal with.

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/js-builtin...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:1755: if (!(search_string =
GetInputStringWitness(node, 2))) return NoChange();
I'm not convinced this is beneficial; can we limit the first version to check
the type of the first parameter against Type::String instead of searching for
witness. I think in 99% of the cases we won't find one and just waste time
searching in the effect chain.

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:1760: CallDescriptor::Flags flags =
CallDescriptor::kNeedsFrameState;
StringIndexOf builtin cannot deopt, so you don't need a frame state here. Will
require some massaging of inputs.

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:1763: descriptor.GetStackParameterCount(),
flags, node->op()->properties());
Also it doesn't have a side effect, so it should use Operator::kEliminatable
here for the properties.

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.cc (right):

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.cc:68: DCHECK_LT(index, parameter_count());
Same, please don't do this in this CL.

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/register-a...
File src/compiler/register-allocator-verifier.cc (right):

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/register-a...
src/compiler/register-allocator-verifier.cc:346: // loopback arc.
Can you revert this change? It's a bit confusing to have a register allocator
change in this CL. You can do it separately if you want.

[Camillo Bruni, 2017-01-19 13:35:18]

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

[commit-bot: I haz the power, 2017-01-19 13:35:28]

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-01-19 13:39:40]

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

[commit-bot: I haz the power, 2017-01-19 13:39:40]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19529)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21149)

[Camillo Bruni, 2017-01-24 14:48:59]

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

[commit-bot: I haz the power, 2017-01-24 14:49:00]

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-01-24 15:23:44]

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

[commit-bot: I haz the power, 2017-01-24 15:23:45]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-01-24 15:51:43]

PTAL again

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/js-builtin...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:1755: if (!(search_string =
GetInputStringWitness(node, 2))) return NoChange();
On 2017/01/18 at 18:10:40, Benedikt Meurer wrote:
> I'm not convinced this is beneficial; can we limit the first version to check
the type of the first parameter against Type::String instead of searching for
witness. I think in 99% of the cases we won't find one and just waste time
searching in the effect chain.

ack. replaced by simple type check,

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:1760: CallDescriptor::Flags flags =
CallDescriptor::kNeedsFrameState;
On 2017/01/18 at 18:10:40, Benedikt Meurer wrote:
> StringIndexOf builtin cannot deopt, so you don't need a frame state here. Will
require some massaging of inputs.

done.

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:1763: descriptor.GetStackParameterCount(),
flags, node->op()->properties());
On 2017/01/18 at 18:10:40, Benedikt Meurer wrote:
> Also it doesn't have a side effect, so it should use Operator::kEliminatable
here for the properties.

done.

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.cc (right):

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.cc:68: DCHECK_LT(index, parameter_count());
On 2017/01/18 at 18:10:40, Benedikt Meurer wrote:
> Same, please don't do this in this CL.

excluded.

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/register-a...
File src/compiler/register-allocator-verifier.cc (right):

https://codereview.chromium.org/2638393002/diff/20001/src/compiler/register-a...
src/compiler/register-allocator-verifier.cc:346: // loopback arc.
On 2017/01/18 at 18:10:41, Benedikt Meurer wrote:
> Can you revert this change? It's a bit confusing to have a register allocator
change in this CL. You can do it separately if you want.

excluded.

[Benedikt Meurer, 2017-01-24 18:12:26]

https://codereview.chromium.org/2638393002/diff/100001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2638393002/diff/100001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:1603: Node* GetInputStringWitness(Node* node,
int input_index) {
Please undo this change as well, since you only check the type of the search
string now.

https://codereview.chromium.org/2638393002/diff/100001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:1753: Operator::kEliminatable);
This should even be pure, since Strings are immutable in JavaScript.

[Camillo Bruni, 2017-01-24 21:30:26]

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

[commit-bot: I haz the power, 2017-01-24 21:30:37]

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-01-24 21:49:01]

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

[commit-bot: I haz the power, 2017-01-24 21:49:01]

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...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Camillo Bruni, 2017-01-25 22:18:01]

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

[commit-bot: I haz the power, 2017-01-25 22:18:10]

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-01-25 22:56:29]

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

[commit-bot: I haz the power, 2017-01-25 22:56:30]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-01-25 23:11:40]

PTAL again.
I somehow randomly tried to massage the arguments and figuring out ways to break
it with tests.

https://codereview.chromium.org/2638393002/diff/100001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2638393002/diff/100001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:1753: Operator::kEliminatable);
On 2017/01/24 at 18:12:26, Benedikt Meurer wrote:
> This should even be pure, since Strings are immutable in JavaScript.

done.

[Benedikt Meurer, 2017-01-26 11:33:01]

Thanks, LGTM

[Camillo Bruni, 2017-01-26 12:44:51]

The CQ bit was checked by cbruni@chromium.org

[commit-bot: I haz the power, 2017-01-26 12:45:02]

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-01-26 12:46:45]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1485434691165000,
"parent_rev": "55aed7821012bc4ffaeeea5d887587e2fc69560f", "commit_rev":
"439a8edcf77947777c5c757b0dec0363d9446093"}

[commit-bot: I haz the power, 2017-01-26 12:46:48]

Description was changed from

==========
[builtins] Add String.prototype.indexOf fast path in TF
==========

to

==========
[builtins] Add String.prototype.indexOf fast path in TF

Review-Url: https://codereview.chromium.org/2638393002
Cr-Commit-Position: refs/heads/master@{#42695}
Committed:
https://chromium.googlesource.com/v8/v8/+/439a8edcf77947777c5c757b0dec0363d94...
==========

[commit-bot: I haz the power, 2017-01-26 12:46:50]

Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/v8/v8/+/439a8edcf77947777c5c757b0dec0363d94...

[Michael Achenbach, 2017-01-27 09:09:00]

Note, this flaky check failures goes back to this CL:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64/builds/15597/ste...