[stubs] Port SubStringStub to TurboFan

[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415
Committed: https://crrev.com/49be31921536716706a6790fbbf9c346b975af16
Committed: https://crrev.com/261d750ea54438089eed71d1b7f57fe741e44ce7
Cr-Original-Commit-Position: refs/heads/master@{#39653}
Cr-Commit-Position: refs/heads/master@{#39851}

[jgruber, 2016-09-20 14:52:26]

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

[commit-bot: I haz the power, 2016-09-20 14:52:31]

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-09-20 14:54:19]

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

[commit-bot: I haz the power, 2016-09-20 14:54:20]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/13091)
  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_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_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  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_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/13053)

[jgruber, 2016-09-20 14:56:09]

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

[commit-bot: I haz the power, 2016-09-20 14:56:19]

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-09-20 14:57:52]

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

[commit-bot: I haz the power, 2016-09-20 14:57:53]

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

[jgruber, 2016-09-20 14:59:02]

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

[commit-bot: I haz the power, 2016-09-20 14:59:09]

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-09-20 15:24:33]

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

[commit-bot: I haz the power, 2016-09-20 15:24:33]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-09-21 09:19:20]

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

[commit-bot: I haz the power, 2016-09-21 09:19:24]

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-09-21 09:46:49]

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

[commit-bot: I haz the power, 2016-09-21 09:46:50]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-09-21 09:55:55]

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

[commit-bot: I haz the power, 2016-09-21 09:55:59]

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

[jgruber, 2016-09-21 09:57:08]

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

[jgruber, 2016-09-21 09:57:11]

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

https://codereview.chromium.org/2355793003/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.cc:2594: // TODO(jgruber): Add an additional case for
substring of length == 0?
Might be worth it to add an additional check here and return the empty_string
root.

https://codereview.chromium.org/2355793003/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.cc:2768: GotoIf(SmiAbove(from,
SmiConstant(Smi::FromInt(0))), &runtime);
This is a behavioral change from the platform runtime stubs.

Platform: %_SubString("abc", 1, 4) -> "abc" since (4 - 1) == "abc".length
Turbofan: %_SubString("abc", 1, 4) -> exception thrown.

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

https://codereview.chromium.org/2355793003/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.h:137: compiler::Node* WordIsPositiveSmi(compiler::Node*
a);
Maybe we should change this function name?

[Benedikt Meurer, 2016-09-21 10:25:00]

Some highlevel comments plus a nit. Will take another look after Igors review.

Can you please file a tracking bug ("Migrate SubStringStub and uses of
SubStringStub to TurboFan"), and be a bit more descriptive about the change;
especially mention the behavioral change in the CL description.

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

https://codereview.chromium.org/2355793003/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:1407: MachineRepresentation::kWord32);
IntPtrConstant vs. kWord32

https://codereview.chromium.org/2355793003/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:1423: IntPtrConstant(String::kEmptyHashField),
IntPtrConstant vs. kWord32

https://codereview.chromium.org/2355793003/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:2547: Node* CodeStubAssembler::StringSubString(Node*
context, Node* string,
Nit: you could rename this to StringSubstr, then it sounds less weird... :-)

[commit-bot: I haz the power, 2016-09-21 10:48:34]

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

[commit-bot: I haz the power, 2016-09-21 10:48:37]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-09-21 11:24:40]

Description was changed from

==========
Port SubString to turbofan stub

BUG=
==========

to

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415
==========

[jgruber, 2016-09-21 11:25:48]

Description was changed from

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415
==========

to

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415
==========

[jgruber, 2016-09-21 11:32:46]

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

[commit-bot: I haz the power, 2016-09-21 11:32:55]

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

[jgruber, 2016-09-21 11:37:58]

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

[jgruber, 2016-09-21 11:38:03]

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

https://codereview.chromium.org/2355793003/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:1407: MachineRepresentation::kWord32);
On 2016/09/21 10:25:00, Benedikt Meurer wrote:
> IntPtrConstant vs. kWord32

Done.

https://codereview.chromium.org/2355793003/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:1423: IntPtrConstant(String::kEmptyHashField),
On 2016/09/21 10:25:00, Benedikt Meurer wrote:
> IntPtrConstant vs. kWord32

Done.

https://codereview.chromium.org/2355793003/diff/80001/src/code-stub-assembler...
src/code-stub-assembler.cc:2547: Node* CodeStubAssembler::StringSubString(Node*
context, Node* string,
On 2016/09/21 10:25:00, Benedikt Meurer wrote:
> Nit: you could rename this to StringSubstr, then it sounds less weird... :-)

Changed to 'SubString' as discussed offline.

[commit-bot: I haz the power, 2016-09-21 11:38:04]

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-09-21 12:05:00]

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

[commit-bot: I haz the power, 2016-09-21 12:05:02]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2016-09-22 12:23:38]

lgtm with nits:

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.cc:2557: var_instance_type.Bind(Int32Constant(0));
You could probably avoid this binding if you move variable definitions after the
underlying_unpacked label definition.

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.h:330: // Allocate a SlicedOneByteString with the given
length, parent and offset.
Add a comment
  // Length and offset are expected to be tagged.
or add ParameterMode parameter.

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.h:334: // Allocate a SlicedTwoByteString with the given
length, parent and offset.
Same here.

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.h:471: // [from,to[ of string.
// |from| and |to| are expected to be tagged here.

https://codereview.chromium.org/2355793003/diff/120001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2355793003/diff/120001/src/code-stubs.cc#newc...
src/code-stubs.cc:2691: // ES6 section 21.1.3.19 String.prototype.substring (
start, end )
It's already section 21.1.3.21

https://codereview.chromium.org/2355793003/diff/120001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2355793003/diff/120001/src/code-stubs.h#newco...
src/code-stubs.h:3141: DEFINE_TURBOFAN_TERNARY_OP_CODE_STUB(SubString,
TurboFanCodeStub);
I don't think it's a ternary operation. It's just a stub that does a part of the
substring job. I think it would be cleaner if you expand this macro here and
give a parameters meaningful names. See FastNewClosureStub for example. 

And it would be better if you also use parameter indices defined in
SubStringDescriptor (see LoadICTrampolineTFStub::GenerateAssembly() for
example).

[jgruber, 2016-09-22 13:21:35]

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

[commit-bot: I haz the power, 2016-09-22 13:21:41]

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

[jgruber, 2016-09-22 13:22:41]

Thanks for the review! Replies inline.

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.cc:2557: var_instance_type.Bind(Int32Constant(0));
On 2016/09/22 12:23:37, Igor Sheludko wrote:
> You could probably avoid this binding if you move variable definitions after
the
> underlying_unpacked label definition.

I'm not so sure - runtime (the label that complained about unbound variables) is
also reachable from underlying_unpacked, so we still have a situation in which
one predecessor of runtime has an uninitialized var_instance_type and others
that don't).

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.h:330: // Allocate a SlicedOneByteString with the given
length, parent and offset.
On 2016/09/22 12:23:37, Igor Sheludko wrote:
> Add a comment
>   // Length and offset are expected to be tagged.
> or add ParameterMode parameter.

Added a comment, it's probably better to hold off on the mode parameter until
it's required somewhere.

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.h:334: // Allocate a SlicedTwoByteString with the given
length, parent and offset.
On 2016/09/22 12:23:37, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2355793003/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.h:471: // [from,to[ of string.
On 2016/09/22 12:23:37, Igor Sheludko wrote:
> // |from| and |to| are expected to be tagged here.

Done.

https://codereview.chromium.org/2355793003/diff/120001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2355793003/diff/120001/src/code-stubs.cc#newc...
src/code-stubs.cc:2691: // ES6 section 21.1.3.19 String.prototype.substring (
start, end )
On 2016/09/22 12:23:37, Igor Sheludko wrote:
> It's already section 21.1.3.21

In ES6 it's still 21.1.3.19:
http://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.substring

https://codereview.chromium.org/2355793003/diff/120001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2355793003/diff/120001/src/code-stubs.h#newco...
src/code-stubs.h:3141: DEFINE_TURBOFAN_TERNARY_OP_CODE_STUB(SubString,
TurboFanCodeStub);
On 2016/09/22 12:23:37, Igor Sheludko wrote:
> I don't think it's a ternary operation. It's just a stub that does a part of
the
> substring job. I think it would be cleaner if you expand this macro here and
> give a parameters meaningful names. See FastNewClosureStub for example. 
> 
> And it would be better if you also use parameter indices defined in
> SubStringDescriptor (see LoadICTrampolineTFStub::GenerateAssembly() for
> example).

Done.

[commit-bot: I haz the power, 2016-09-22 13:55:11]

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

[commit-bot: I haz the power, 2016-09-22 13:55:13]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-09-22 17:55:18]

Thanks. LGTM from my side.

[jgruber, 2016-09-23 06:47:21]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-09-23 06:47:21]

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

[commit-bot: I haz the power, 2016-09-23 06:47:33]

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-09-23 06:50:20]

Description was changed from

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415
==========

to

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415
==========

[commit-bot: I haz the power, 2016-09-23 06:50:21]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-23 06:50:41]

Description was changed from

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415
==========

to

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415

Committed: https://crrev.com/49be31921536716706a6790fbbf9c346b975af16
Cr-Commit-Position: refs/heads/master@{#39653}
==========

[commit-bot: I haz the power, 2016-09-23 06:50:42]

Patchset 8 (id:??) landed as
https://crrev.com/49be31921536716706a6790fbbf9c346b975af16
Cr-Commit-Position: refs/heads/master@{#39653}

[Michael Hablich, 2016-09-26 14:32:06]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2365413002/ by hablich@chromium.org.

The reason for reverting is: Speculative revert because of stability problems.

[jgruber, 2016-09-27 13:25:29]

Description was changed from

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415

Committed: https://crrev.com/49be31921536716706a6790fbbf9c346b975af16
Cr-Commit-Position: refs/heads/master@{#39653}
==========

to

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415

Committed: https://crrev.com/49be31921536716706a6790fbbf9c346b975af16
Cr-Commit-Position: refs/heads/master@{#39653}
==========

[jgruber, 2016-09-27 13:25:47]

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

[commit-bot: I haz the power, 2016-09-27 13:25: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, 2016-09-27 13:27:25]

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

[commit-bot: I haz the power, 2016-09-27 13:27:26]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/13546)
  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/...)

[jgruber, 2016-09-27 13:46:30]

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

[commit-bot: I haz the power, 2016-09-27 13:46:41]

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-09-27 13:50:10]

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

[commit-bot: I haz the power, 2016-09-27 13:50:12]

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

[jgruber, 2016-09-27 14:10:30]

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

[commit-bot: I haz the power, 2016-09-27 14:10:46]

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

[jgruber, 2016-09-27 14:11:16]

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

[commit-bot: I haz the power, 2016-09-27 14:11: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-09-27 16:11:44]

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

[commit-bot: I haz the power, 2016-09-27 16:11:47]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)

[jgruber, 2016-09-28 08:50:39]

PTAL. This fixes the recent canary crash by never storing the pointer into the
external string in a tagged variable, and instead inlining allocation +
character copy logic for both external and non-external cases.

[Igor Sheludko, 2016-09-28 16:04:26]

lgtm with nits:

https://codereview.chromium.org/2355793003/diff/240001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2355793003/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:1708: Node* byte_count =
SmiToWord32(character_count);
Do SmiUntag() instead or we will end up mixing int_ptr/int32 computations.

https://codereview.chromium.org/2355793003/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:1709: Node* from_byte_index =
SmiToWord32(from_index);
Same here.

[jgruber, 2016-09-29 07:06:41]

https://codereview.chromium.org/2355793003/diff/240001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2355793003/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:1708: Node* byte_count =
SmiToWord32(character_count);
On 2016/09/28 16:04:26, Igor Sheludko wrote:
> Do SmiUntag() instead or we will end up mixing int_ptr/int32 computations.

Done.

https://codereview.chromium.org/2355793003/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:1709: Node* from_byte_index =
SmiToWord32(from_index);
On 2016/09/28 16:04:26, Igor Sheludko wrote:
> Same here.

Done.

[jgruber, 2016-09-29 07:06:47]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-09-29 07:06:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2355793003/#ps260001
(title: "Address comments")

[commit-bot: I haz the power, 2016-09-29 07:06:59]

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-09-29 07:34:49]

Description was changed from

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415

Committed: https://crrev.com/49be31921536716706a6790fbbf9c346b975af16
Cr-Commit-Position: refs/heads/master@{#39653}
==========

to

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415

Committed: https://crrev.com/49be31921536716706a6790fbbf9c346b975af16
Cr-Commit-Position: refs/heads/master@{#39653}
==========

[commit-bot: I haz the power, 2016-09-29 07:34:51]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-09-29 07:35:11]

Description was changed from

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415

Committed: https://crrev.com/49be31921536716706a6790fbbf9c346b975af16
Cr-Commit-Position: refs/heads/master@{#39653}
==========

to

==========
[stubs] Port SubStringStub to TurboFan

This ports the platform-specific SubStringStub to TurboFan.

It also contains a minor bug-fix for the case when the requested substring
length equals the subject string length, but the start index is not equal to 0.
The old stub implementation returned the subject string, while the new
implementation calls into runtime, which finally results in a thrown exception.

BUG=v8:5415

Committed: https://crrev.com/49be31921536716706a6790fbbf9c346b975af16
Committed: https://crrev.com/261d750ea54438089eed71d1b7f57fe741e44ce7
Cr-Original-Commit-Position: refs/heads/master@{#39653}
Cr-Commit-Position: refs/heads/master@{#39851}
==========

[commit-bot: I haz the power, 2016-09-29 07:35:12]

Patchset 14 (id:??) landed as
https://crrev.com/261d750ea54438089eed71d1b7f57fe741e44ce7
Cr-Commit-Position: refs/heads/master@{#39851}