[turbofan] inline %StringIteratorPrototype%.next in JSBuiltinReducer.

[turbofan] inline %StringIteratorPrototype%.next in JSBuiltinReducer.

Implement the logic for StringIterator.prototype.next in the JSBuiltinReducer in order to allow inlining when the receiver is a JS_STRING_ITERATOR_TYPE map, built ontop of the SimplifiedOperators StringCharCodeAt and the newly added StringFromCodePoint.

Also introduces a new StringFromCodePoint simplified op which may be useful for other String builtins, such as String.fromCodePoint()

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org

Committed: https://crrev.com/aed32e0f22353527993de8bceaf246fc744558f5
Cr-Commit-Position: refs/heads/master@{#39994}

[caitp, 2016-09-29 12:35:22]

caitp@igalia.com changed reviewers:
+ bmeurer@chromium.org, jwolfe@igalia.com

[caitp, 2016-09-29 12:37:27]

Hi,

I'm having some trouble with this --- I think I've sorted out most of the Node
verifier stuff, but now the scheduler is failing, and it's challenging to find
the cause as there is so much code being produced here. --trace-turbo-scheduler
doesn't really narrow down the problem area that much :x

CC'd Benedikt due to knowledge, but Josh should be able to help with this.

[Benedikt Meurer, 2016-09-29 13:04:27]

First round of comments; need to dive into this to help you with the scheduler.

https://codereview.chromium.org/2373983004/diff/1/src/compiler/access-builder.cc
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2373983004/diff/1/src/compiler/access-builder...
src/compiler/access-builder.cc:555: Type::String(),
MachineType::TaggedPointer(),    kNoWriteBarrier};
This should be kPointerWriteBarrier.

https://codereview.chromium.org/2373983004/diff/1/src/compiler/access-builder...
src/compiler/access-builder.cc:563: Handle<String>(),     Type::Signed32(),
The type of this field is Type::UnsignedSmall(), as it can only contain positive
Smis. Actually it can only contain Smis in the range [0, String::kMaxLength], so
this should be TypeCache::Get().kStringLengthType instead.

https://codereview.chromium.org/2373983004/diff/1/src/compiler/access-builder...
src/compiler/access-builder.cc:564: MachineType::Int32(), kNoWriteBarrier};
This should be MachineType::TaggedSigned() instead of MachineType::Int32().

https://codereview.chromium.org/2373983004/diff/1/src/compiler/effect-control...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2373983004/diff/1/src/compiler/effect-control...
src/compiler/effect-control-linearizer.cc:2234:
EffectControlLinearizer::LowerStringCharCodeAt(Node* node, Node* effect,
Can you hold off with changing the StringCharCodeAt operator? Let's keep it
simple first and use that one as-is.

https://codereview.chromium.org/2373983004/diff/1/src/compiler/js-builtin-red...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2373983004/diff/1/src/compiler/js-builtin-red...
src/compiler/js-builtin-reducer.cc:1051: Node* etrue0 = effect;
You don't need the etrue0.

https://codereview.chromium.org/2373983004/diff/1/src/compiler/js-builtin-red...
src/compiler/js-builtin-reducer.cc:1054: Node* done_true;
Can you indent the branches below and add { } for readability? I have a hard
time following the control flow.

https://codereview.chromium.org/2373983004/diff/1/src/compiler/js-builtin-red...
src/compiler/js-builtin-reducer.cc:1057: Node* uint32_index =
You don't need this NumberToUint32. The type of the index field should be
Type::UnsignedSmall() and thus make the verifier and everyone happy. :-)

https://codereview.chromium.org/2373983004/diff/1/src/compiler/js-builtin-red...
src/compiler/js-builtin-reducer.cc:1084:
graph()->NewNode(simplified()->NumberToUint32(), next_index);
Same here, you don't need it if you have the types right.

https://codereview.chromium.org/2373983004/diff/1/src/compiler/js-builtin-red...
src/compiler/js-builtin-reducer.cc:1132: vtrue0 = etrue0 = graph()->NewNode(
StringFromCodePoint doesn't produce an effect.

https://codereview.chromium.org/2373983004/diff/1/src/compiler/js-builtin-red...
src/compiler/js-builtin-reducer.cc:1165: value, map, effect, control);
You need to initialize properties and elements here as well (to the empty fixed
array).

https://codereview.chromium.org/2373983004/diff/1/src/compiler/simplified-ope...
File src/compiler/simplified-operator.h (right):

https://codereview.chromium.org/2373983004/diff/1/src/compiler/simplified-ope...
src/compiler/simplified-operator.h:188: enum class StringOperationHint : uint8_t
{
Please remove this optimization from this CL. Let's look at that later.

https://codereview.chromium.org/2373983004/diff/1/src/compiler/verifier.cc
File src/compiler/verifier.cc (right):

https://codereview.chromium.org/2373983004/diff/1/src/compiler/verifier.cc#ne...
src/compiler/verifier.cc:850: // (Unsigned32) -> String
Nit: Number -> String

https://codereview.chromium.org/2373983004/diff/1/src/heap/slot-set.h
File src/heap/slot-set.h (right):

https://codereview.chromium.org/2373983004/diff/1/src/heap/slot-set.h#newcode328
src/heap/slot-set.h:328: FreeToBeFreedChunks();
What is this?

https://codereview.chromium.org/2373983004/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2373983004/diff/1/src/objects.h#newcode7029
src/objects.h:7029: V(% StringIteratorPrototype %, next,
StringIteratorPrototypeNext) \
Don't add it here, but to the special list below at the end of the enum.

[Benedikt Meurer, 2016-09-29 16:40:01]

https://codereview.chromium.org/2373983004/diff/1/src/compiler/effect-control...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2373983004/diff/1/src/compiler/effect-control...
src/compiler/effect-control-linearizer.cc:2234:
EffectControlLinearizer::LowerStringCharCodeAt(Node* node, Node* effect,
I wonder how much the pre-flattening actually buys us. Can you measure with the
baseline version? If it doesn't buy us much, i.e. we just do the full
StringCharCodeAt in every iteration, then I'd prefer to not do it at all, and go
for the simple version (maximizing code sharing in this case).

[Benedikt Meurer, 2016-09-29 17:19:05]

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

https://codereview.chromium.org/2373983004/diff/20001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:1147: { done_false = vfalse0 =
jsgraph()->UndefinedConstant(); }
You should really set done to False in this case, otherwise scalar replacement
will not get rid of the ToBoolean on the .done access for us; plus it violates
the spec.

[caitp, 2016-09-29 17:21:45]

https://codereview.chromium.org/2373983004/diff/1/src/compiler/js-builtin-red...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2373983004/diff/1/src/compiler/js-builtin-red...
src/compiler/js-builtin-reducer.cc:1057: Node* uint32_index =
On 2016/09/29 13:04:27, Benedikt Meurer wrote:
> You don't need this NumberToUint32. The type of the index field should be
> Type::UnsignedSmall() and thus make the verifier and everyone happy. :-)

Ah, makes sense. Thanks!

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

https://codereview.chromium.org/2373983004/diff/20001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:1147: { done_false = vfalse0 =
jsgraph()->UndefinedConstant(); }
On 2016/09/29 17:19:05, Benedikt Meurer wrote:
> You should really set done to False in this case, otherwise scalar replacement
> will not get rid of the ToBoolean on the .done access for us; plus it violates
> the spec.

Er, something went wrong here --- this is supposed be `done_false =
jsgraph()->FalseConstant();`. Not sure what happened there, but noted, will fix
that in the next version. Good eye.

[caitp, 2016-09-29 20:08:02]

https://codereview.chromium.org/2373983004/diff/1/src/compiler/effect-control...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2373983004/diff/1/src/compiler/effect-control...
src/compiler/effect-control-linearizer.cc:2234:
EffectControlLinearizer::LowerStringCharCodeAt(Node* node, Node* effect,
On 2016/09/29 16:40:01, Benedikt Meurer wrote:
> I wonder how much the pre-flattening actually buys us. Can you measure with
the
> baseline version? If it doesn't buy us much, i.e. we just do the full
> StringCharCodeAt in every iteration, then I'd prefer to not do it at all, and
go
> for the simple version (maximizing code sharing in this case).

Comparing these, it doesn't look like there's a significant performance
difference between:

```
tools/run_perf.py --arch x64 --outdir=out --outdir-no-patch=tf_stringiter/
test/js-perf-test/JSTests.json 
>>> Running suite: JSTests/StringIterators
>>> Stdout (#1):
Spread_OneByteShort-StringIterators(Score): 151860
Spread_TwoByteShort-StringIterators(Score): 303680
Spread_WithSurrogatePairsShort-StringIterators(Score): 229080
ForOf_OneByteShort-StringIterators(Score): 261070
ForOf_TwoByteShort-StringIterators(Score): 572460
ForOf_WithSurrogatePairsShort-StringIterators(Score): 420630
ForOf_OneByteLong-StringIterators(Score): 2390
ForOf_TwoByteLong-StringIterators(Score): 2512
ForOf_WithSurrogatePairsLong-StringIterators(Score): 3776

>>> Stdout (#1) - without patch:
Spread_OneByteShort-StringIterators(Score): 148900
Spread_TwoByteShort-StringIterators(Score): 296180
Spread_WithSurrogatePairsShort-StringIterators(Score): 234100
ForOf_OneByteShort-StringIterators(Score): 270310
ForOf_TwoByteShort-StringIterators(Score): 610690
ForOf_WithSurrogatePairsShort-StringIterators(Score): 430120
ForOf_OneByteLong-StringIterators(Score): 2250
ForOf_TwoByteLong-StringIterators(Score): 2361
ForOf_WithSurrogatePairsLong-StringIterators(Score): 3760

>>> Stdout (#2):
Spread_OneByteShort-StringIterators(Score): 141610
Spread_TwoByteShort-StringIterators(Score): 295680
Spread_WithSurrogatePairsShort-StringIterators(Score): 237640
ForOf_OneByteShort-StringIterators(Score): 275040
ForOf_TwoByteShort-StringIterators(Score): 604910
ForOf_WithSurrogatePairsShort-StringIterators(Score): 405770
ForOf_OneByteLong-StringIterators(Score): 2438
ForOf_TwoByteLong-StringIterators(Score): 2453
ForOf_WithSurrogatePairsLong-StringIterators(Score): 3730

>>> Stdout (#2) - without patch:
Spread_OneByteShort-StringIterators(Score): 145840
Spread_TwoByteShort-StringIterators(Score): 299810
Spread_WithSurrogatePairsShort-StringIterators(Score): 236650
ForOf_OneByteShort-StringIterators(Score): 270420
ForOf_TwoByteShort-StringIterators(Score): 552390
ForOf_WithSurrogatePairsShort-StringIterators(Score): 420850
ForOf_OneByteLong-StringIterators(Score): 2380
ForOf_TwoByteLong-StringIterators(Score): 2428
ForOf_WithSurrogatePairsLong-StringIterators(Score): 3832

>>> Stdout (#3):
Spread_OneByteShort-StringIterators(Score): 150870
Spread_TwoByteShort-StringIterators(Score): 295960
Spread_WithSurrogatePairsShort-StringIterators(Score): 237950
ForOf_OneByteShort-StringIterators(Score): 274050
ForOf_TwoByteShort-StringIterators(Score): 591230
ForOf_WithSurrogatePairsShort-StringIterators(Score): 424490
ForOf_OneByteLong-StringIterators(Score): 2343
ForOf_TwoByteLong-StringIterators(Score): 2475
ForOf_WithSurrogatePairsLong-StringIterators(Score): 3776

>>> Stdout (#3) - without patch:
Spread_OneByteShort-StringIterators(Score): 147860
Spread_TwoByteShort-StringIterators(Score): 304420
Spread_WithSurrogatePairsShort-StringIterators(Score): 233880
ForOf_OneByteShort-StringIterators(Score): 268870
ForOf_TwoByteShort-StringIterators(Score): 600230
ForOf_WithSurrogatePairsShort-StringIterators(Score): 436650
ForOf_OneByteLong-StringIterators(Score): 2353
ForOf_TwoByteLong-StringIterators(Score): 2430
ForOf_WithSurrogatePairsLong-StringIterators(Score): 3860
```

It's pretty close to neutral, so I don't mind sending a patch to switch to the
unflattening model

[Benedikt Meurer, 2016-09-30 04:23:19]

On 2016/09/29 20:08:02, caitp wrote:
>
https://codereview.chromium.org/2373983004/diff/1/src/compiler/effect-control...
> File src/compiler/effect-control-linearizer.cc (right):
> 
>
https://codereview.chromium.org/2373983004/diff/1/src/compiler/effect-control...
> src/compiler/effect-control-linearizer.cc:2234:
> EffectControlLinearizer::LowerStringCharCodeAt(Node* node, Node* effect,
> On 2016/09/29 16:40:01, Benedikt Meurer wrote:
> > I wonder how much the pre-flattening actually buys us. Can you measure with
> the
> > baseline version? If it doesn't buy us much, i.e. we just do the full
> > StringCharCodeAt in every iteration, then I'd prefer to not do it at all,
and
> go
> > for the simple version (maximizing code sharing in this case).
> 
> Comparing these, it doesn't look like there's a significant performance
> difference between:
> 
> ```
> tools/run_perf.py --arch x64 --outdir=out --outdir-no-patch=tf_stringiter/
> test/js-perf-test/JSTests.json 
> >>> Running suite: JSTests/StringIterators
> >>> Stdout (#1):
> Spread_OneByteShort-StringIterators(Score): 151860
> Spread_TwoByteShort-StringIterators(Score): 303680
> Spread_WithSurrogatePairsShort-StringIterators(Score): 229080
> ForOf_OneByteShort-StringIterators(Score): 261070
> ForOf_TwoByteShort-StringIterators(Score): 572460
> ForOf_WithSurrogatePairsShort-StringIterators(Score): 420630
> ForOf_OneByteLong-StringIterators(Score): 2390
> ForOf_TwoByteLong-StringIterators(Score): 2512
> ForOf_WithSurrogatePairsLong-StringIterators(Score): 3776
> 
> >>> Stdout (#1) - without patch:
> Spread_OneByteShort-StringIterators(Score): 148900
> Spread_TwoByteShort-StringIterators(Score): 296180
> Spread_WithSurrogatePairsShort-StringIterators(Score): 234100
> ForOf_OneByteShort-StringIterators(Score): 270310
> ForOf_TwoByteShort-StringIterators(Score): 610690
> ForOf_WithSurrogatePairsShort-StringIterators(Score): 430120
> ForOf_OneByteLong-StringIterators(Score): 2250
> ForOf_TwoByteLong-StringIterators(Score): 2361
> ForOf_WithSurrogatePairsLong-StringIterators(Score): 3760
> 
> >>> Stdout (#2):
> Spread_OneByteShort-StringIterators(Score): 141610
> Spread_TwoByteShort-StringIterators(Score): 295680
> Spread_WithSurrogatePairsShort-StringIterators(Score): 237640
> ForOf_OneByteShort-StringIterators(Score): 275040
> ForOf_TwoByteShort-StringIterators(Score): 604910
> ForOf_WithSurrogatePairsShort-StringIterators(Score): 405770
> ForOf_OneByteLong-StringIterators(Score): 2438
> ForOf_TwoByteLong-StringIterators(Score): 2453
> ForOf_WithSurrogatePairsLong-StringIterators(Score): 3730
> 
> >>> Stdout (#2) - without patch:
> Spread_OneByteShort-StringIterators(Score): 145840
> Spread_TwoByteShort-StringIterators(Score): 299810
> Spread_WithSurrogatePairsShort-StringIterators(Score): 236650
> ForOf_OneByteShort-StringIterators(Score): 270420
> ForOf_TwoByteShort-StringIterators(Score): 552390
> ForOf_WithSurrogatePairsShort-StringIterators(Score): 420850
> ForOf_OneByteLong-StringIterators(Score): 2380
> ForOf_TwoByteLong-StringIterators(Score): 2428
> ForOf_WithSurrogatePairsLong-StringIterators(Score): 3832
> 
> >>> Stdout (#3):
> Spread_OneByteShort-StringIterators(Score): 150870
> Spread_TwoByteShort-StringIterators(Score): 295960
> Spread_WithSurrogatePairsShort-StringIterators(Score): 237950
> ForOf_OneByteShort-StringIterators(Score): 274050
> ForOf_TwoByteShort-StringIterators(Score): 591230
> ForOf_WithSurrogatePairsShort-StringIterators(Score): 424490
> ForOf_OneByteLong-StringIterators(Score): 2343
> ForOf_TwoByteLong-StringIterators(Score): 2475
> ForOf_WithSurrogatePairsLong-StringIterators(Score): 3776
> 
> >>> Stdout (#3) - without patch:
> Spread_OneByteShort-StringIterators(Score): 147860
> Spread_TwoByteShort-StringIterators(Score): 304420
> Spread_WithSurrogatePairsShort-StringIterators(Score): 233880
> ForOf_OneByteShort-StringIterators(Score): 268870
> ForOf_TwoByteShort-StringIterators(Score): 600230
> ForOf_WithSurrogatePairsShort-StringIterators(Score): 436650
> ForOf_OneByteLong-StringIterators(Score): 2353
> ForOf_TwoByteLong-StringIterators(Score): 2430
> ForOf_WithSurrogatePairsLong-StringIterators(Score): 3860
> ```
> 
> It's pretty close to neutral, so I don't mind sending a patch to switch to the
> unflattening model

Awesome, please do that. I like the simplicity of that approach.

[caitp, 2016-10-01 01:20:13]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 01:20:17]

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

[caitp, 2016-10-01 01:20:51]

Description was changed from

==========
WIP %StringPrototypeIterator%.next inlining
==========

to

==========
BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
==========

[caitp, 2016-10-01 01:31:12]

I've done some work on this (thanks for the help with the scheduler error), and
I believe this passes tests (at least does the right thing in some simple
cases).

In the in-tree benchmarks, it's difficult to see any improvement caused by this,
but running the benchmark manually with `--turbo --turbo-inlining
--trace-turbo-inlining --trace-turbo-reduction` indicates that these benchmarks
probably don't benefit from this reducer anyways. It would be nice to have a
test that would see some non-neutral difference.

[caitp, 2016-10-01 01:36:07]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 01:36:15]

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-10-01 01:38:25]

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

[commit-bot: I haz the power, 2016-10-01 01:38: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_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/13788)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10013)

[caitp, 2016-10-01 01:39:08]

On 2016/10/01 01:31:12, caitp wrote:
> I've done some work on this (thanks for the help with the scheduler error),
and
> I believe this passes tests (at least does the right thing in some simple
> cases).
> 
> In the in-tree benchmarks, it's difficult to see any improvement caused by
this,
> but running the benchmark manually with `--turbo --turbo-inlining
> --trace-turbo-inlining --trace-turbo-reduction` indicates that these
benchmarks
> probably don't benefit from this reducer anyways. It would be nice to have a
> test that would see some non-neutral difference.

I stand corrected, I guess weird things are happening for some reason. Will
investigate that later.

[caitp, 2016-10-01 10:33:53]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 10:34:03]

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-10-01 10:38:12]

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

[commit-bot: I haz the power, 2016-10-01 10:38:13]

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

[caitp, 2016-10-01 10:49:39]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 10:49:43]

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-10-01 10:54:32]

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

[commit-bot: I haz the power, 2016-10-01 10:54:33]

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

[caitp, 2016-10-01 10:56:11]

On 2016/10/01 01:39:08, caitp wrote:
> On 2016/10/01 01:31:12, caitp wrote:
> > I've done some work on this (thanks for the help with the scheduler error),
> and
> > I believe this passes tests (at least does the right thing in some simple
> > cases).
> > 
> > In the in-tree benchmarks, it's difficult to see any improvement caused by
> this,
> > but running the benchmark manually with `--turbo --turbo-inlining
> > --trace-turbo-inlining --trace-turbo-reduction` indicates that these
> benchmarks
> > probably don't benefit from this reducer anyways. It would be nice to have a
> > test that would see some non-neutral difference.
> 
> I stand corrected, I guess weird things are happening for some reason. Will
> investigate that later.

I have no idea what's causing it :( it's very strange

[caitp, 2016-10-01 17:34:58]

https://codereview.chromium.org/2373983004/diff/110001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2373983004/diff/110001/src/objects.h#newcode7036
src/objects.h:7036: V(% StringIteratorPrototype %, next,
StringIteratorPrototypeNext) \
Mystery solved

[Benedikt Meurer, 2016-10-01 17:36:25]

https://codereview.chromium.org/2373983004/diff/110001/src/compiler/access-bu...
File src/compiler/access-builder.h (right):

https://codereview.chromium.org/2373983004/diff/110001/src/compiler/access-bu...
src/compiler/access-builder.h:178: static FieldAccess
ForSeqTwoByteStringFirstCharacterPair();
Remove this unused method.

https://codereview.chromium.org/2373983004/diff/110001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2373983004/diff/110001/src/objects.h#newcode7036
src/objects.h:7036: V(% StringIteratorPrototype %, next,
StringIteratorPrototypeNext) \
Yeah, please move this down to the special ones and name it "StringIteratorNext"
to match the naming convention (which is argubly bogus, but then its at least
consistent).

[caitp, 2016-10-01 17:37:41]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 17:37:51]

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

[Benedikt Meurer, 2016-10-01 17:41:03]

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

https://codereview.chromium.org/2373983004/diff/130001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:1167:
jsgraph()->Int32Constant(JSIteratorResult::kSize), effect, control);
With your Typer change, you'll need to have a compatible type on this Allocate
as well, otherwise the verifier won't like you, i.e. insert a line

NodeProperties::SetType(value, Type::OtherObject());

here.

https://codereview.chromium.org/2373983004/diff/130001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/2373983004/diff/130001/src/compiler/typer.cc#...
src/compiler/typer.cc:1371: return Type::DetectableReceiver();
Awesome, I was just about to point this out. However, this should be
Type::OtherObject, because we know its not a function or a proxy.

https://codereview.chromium.org/2373983004/diff/130001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2373983004/diff/130001/src/objects.h#newcode7109
src/objects.h:7109: kStringIteratorPrototypeNext,
Please change name to kStringIteratorNext to match naming convention here.

[Benedikt Meurer, 2016-10-01 17:59:33]

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

https://codereview.chromium.org/2373983004/diff/130001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:1167:
jsgraph()->Int32Constant(JSIteratorResult::kSize), effect, control);
Actually I'd be even better to just use the existing JSCreateIterResultObject
operator here, so we don't need to have this code N times. I.e. something along
the lines of

https://gist.github.com/bmeurer/f2a6b4d95e5d9ee39852cfa138ac7319

This also addresses a bug you have in the Allocate, where you pass an
Int32Constant instead of a NumberConstant (created via jsgraph()->Constant),
which blows up with --turbo-escape.

[caitp, 2016-10-01 18:02:43]

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

https://codereview.chromium.org/2373983004/diff/130001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:1167:
jsgraph()->Int32Constant(JSIteratorResult::kSize), effect, control);
On 2016/10/01 17:59:33, Benedikt Meurer wrote:
> Actually I'd be even better to just use the existing JSCreateIterResultObject
> operator here, so we don't need to have this code N times. I.e. something
along
> the lines of
> 
> https://gist.github.com/bmeurer/f2a6b4d95e5d9ee39852cfa138ac7319
> 
> This also addresses a bug you have in the Allocate, where you pass an
> Int32Constant instead of a NumberConstant (created via jsgraph()->Constant),
> which blows up with --turbo-escape.

Oh, I didn't realize there was an operator for this already --- sounds good to
me. This should allow omitting the BeginRegion()/FinishRegion() nodes as well,
right?

edit --- I guess your patch does omit those, cool.

https://codereview.chromium.org/2373983004/diff/130001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2373983004/diff/130001/src/objects.h#newcode7109
src/objects.h:7109: kStringIteratorPrototypeNext,
On 2016/10/01 17:41:03, Benedikt Meurer wrote:
> Please change name to kStringIteratorNext to match naming convention here.

done

[Benedikt Meurer, 2016-10-01 18:18:31]

Looks pretty good already, and tried it locally and the code looks pretty
optimal now as far as the .next is concerned. I have to take another look, but
it seems we almost reached the point where the remaining optimizations have to
happen in the escape analysis now to make this awesome.

Will do an in-depth review next week.

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

https://codereview.chromium.org/2373983004/diff/130001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:1167:
jsgraph()->Int32Constant(JSIteratorResult::kSize), effect, control);
Yes, you just pass in value and done (plus context and effect), and it does
everything for you. More code sharing :-)

[Benedikt Meurer, 2016-10-04 09:28:21]

Description was changed from

==========
BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
==========

to

==========
[turbofan] inline %StringIteratorPrototype%.next in JSBuiltinReducer.

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
==========

[Benedikt Meurer, 2016-10-04 09:28:46]

Please go into some detail in the CL description :-)

[Benedikt Meurer, 2016-10-04 09:53:59]

bmeurer@chromium.org changed reviewers:
+ mvstanton@chromium.org
- jwolfe@igalia.com

[Benedikt Meurer, 2016-10-04 09:53:59]

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

[Benedikt Meurer, 2016-10-04 09:54:00]

LGTM from my side (once CL description is more detailed). Thanks a lot!

[caitp, 2016-10-04 10:00:01]

On 2016/10/04 09:54:00, Benedikt Meurer wrote:
> LGTM from my side (once CL description is more detailed). Thanks a lot!

Should I get a look from anyone else? I'll be unavailable for the next day or
so, so I won't be around for a quick fix if it causes a regression somewhere.

[caitp, 2016-10-04 10:07:32]

Description was changed from

==========
[turbofan] inline %StringIteratorPrototype%.next in JSBuiltinReducer.

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
==========

to

==========
[turbofan] inline %StringIteratorPrototype%.next in JSBuiltinReducer.

Implement the logic for StringIterator.prototype.next in the JSBuiltinReducer in
order to allow inlining when the receiver is a JS_STRING_ITERATOR_TYPE map,
built ontop of the SimplifiedOperators StringCharCodeAt and the newly added
StringFromCodePoint.

Also introduces a new StringFromCodePoint simplified op which may be useful for
other String builtins, such as String.fromCodePoint()


BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
==========

[caitp, 2016-10-04 10:12:56]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 10:13:04]

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

[Benedikt Meurer, 2016-10-04 10:13:48]

Michael will do a review. It's fine if you don't land it today, we're not in a
hurry and surrounding code won't change a lot (probably not at all).

[commit-bot: I haz the power, 2016-10-04 10:42:55]

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

[commit-bot: I haz the power, 2016-10-04 10:42:55]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2016-10-05 07:58:52]

Really nice! One nit to fix then LGTM.

https://codereview.chromium.org/2373983004/diff/190001/src/compiler/effect-co...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2373983004/diff/190001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.cc:2712: break;
Remove extra break statement here.

[caitp, 2016-10-05 12:36:37]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 12:36:44]

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-10-05 13:02:55]

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

[commit-bot: I haz the power, 2016-10-05 13:02:56]

Dry run: This issue passed the CQ dry run.

[caitp, 2016-10-05 13:09:22]

The CQ bit was checked by caitp@igalia.com

[caitp, 2016-10-05 13:09:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mvstanton@chromium.org
Link to the patchset: https://codereview.chromium.org/2373983004/#ps210001
(title: "some renaming for consistency + remove the extra break")

[commit-bot: I haz the power, 2016-10-05 13:09:32]

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

[caitp, 2016-10-05 13:09:55]

https://codereview.chromium.org/2373983004/diff/190001/src/compiler/effect-co...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2373983004/diff/190001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.cc:2712: break;
On 2016/10/05 07:58:52, mvstanton wrote:
> Remove extra break statement here.

Oops, done

[commit-bot: I haz the power, 2016-10-05 13:12:31]

Description was changed from

==========
[turbofan] inline %StringIteratorPrototype%.next in JSBuiltinReducer.

Implement the logic for StringIterator.prototype.next in the JSBuiltinReducer in
order to allow inlining when the receiver is a JS_STRING_ITERATOR_TYPE map,
built ontop of the SimplifiedOperators StringCharCodeAt and the newly added
StringFromCodePoint.

Also introduces a new StringFromCodePoint simplified op which may be useful for
other String builtins, such as String.fromCodePoint()


BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
==========

to

==========
[turbofan] inline %StringIteratorPrototype%.next in JSBuiltinReducer.

Implement the logic for StringIterator.prototype.next in the JSBuiltinReducer in
order to allow inlining when the receiver is a JS_STRING_ITERATOR_TYPE map,
built ontop of the SimplifiedOperators StringCharCodeAt and the newly added
StringFromCodePoint.

Also introduces a new StringFromCodePoint simplified op which may be useful for
other String builtins, such as String.fromCodePoint()


BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
==========

[commit-bot: I haz the power, 2016-10-05 13:12:32]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-10-05 13:12:53]

Description was changed from

==========
[turbofan] inline %StringIteratorPrototype%.next in JSBuiltinReducer.

Implement the logic for StringIterator.prototype.next in the JSBuiltinReducer in
order to allow inlining when the receiver is a JS_STRING_ITERATOR_TYPE map,
built ontop of the SimplifiedOperators StringCharCodeAt and the newly added
StringFromCodePoint.

Also introduces a new StringFromCodePoint simplified op which may be useful for
other String builtins, such as String.fromCodePoint()


BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
==========

to

==========
[turbofan] inline %StringIteratorPrototype%.next in JSBuiltinReducer.

Implement the logic for StringIterator.prototype.next in the JSBuiltinReducer in
order to allow inlining when the receiver is a JS_STRING_ITERATOR_TYPE map,
built ontop of the SimplifiedOperators StringCharCodeAt and the newly added
StringFromCodePoint.

Also introduces a new StringFromCodePoint simplified op which may be useful for
other String builtins, such as String.fromCodePoint()

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org

Committed: https://crrev.com/aed32e0f22353527993de8bceaf246fc744558f5
Cr-Commit-Position: refs/heads/master@{#39994}
==========

[commit-bot: I haz the power, 2016-10-05 13:12:55]

Patchset 12 (id:??) landed as
https://crrev.com/aed32e0f22353527993de8bceaf246fc744558f5
Cr-Commit-Position: refs/heads/master@{#39994}

[Michael Achenbach, 2016-10-05 14:29:22]

A revert of this CL (patchset #12 id:210001) has been created in
https://codereview.chromium.org/2397753003/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Speculative revert for win dbg:
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds...

Or we have an infra problem. Manual build before seems fine:
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds....

[caitp, 2016-10-05 17:47:24]

On 2016/10/05 14:29:22, machenbach (slow) wrote:
> A revert of this CL (patchset #12 id:210001) has been created in
> https://codereview.chromium.org/2397753003/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: [Sheriff] Speculative revert for win dbg:
>
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds...
> 
> Or we have an infra problem. Manual build before seems fine:
>
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds....

The Win32 Debug failiures appear to be unrelated to this CL. A reland CL is up
at https://codereview.chromium.org/2394823003

[Michael Achenbach, 2016-10-05 18:03:15]

On 2016/10/05 17:47:24, caitp wrote:
> On 2016/10/05 14:29:22, machenbach (slow) wrote:
> > A revert of this CL (patchset #12 id:210001) has been created in
> > https://codereview.chromium.org/2397753003/ by
mailto:machenbach@chromium.org.
> > 
> > The reason for reverting is: [Sheriff] Speculative revert for win dbg:
> >
>
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds...
> > 
> > Or we have an infra problem. Manual build before seems fine:
> >
>
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds....
> 
> The Win32 Debug failiures appear to be unrelated to this CL. A reland CL is up
> at https://codereview.chromium.org/2394823003

Yes. We seem to have an incremental build problem. Just re land. Sorry...