[turbofan] Implement JSBuiltinReducer for String.prototype[Symbol.iterator].

[turbofan] Implement JSBuiltinReducer for String.prototype[Symbol.iterator].

BUG=v8:5388
Committed: https://crrev.com/21fda06c05e26b9085227682495248532fbb8357
Cr-Commit-Position: refs/heads/master@{#40406}

[jwolfe, 2016-10-17 21:40:06]

Description was changed from

==========
Implement JSBuiltinReducer for String.prototype[Symbol.iterator]

Known issues: these tests fail:
* cctest/test-run-bytecode-graph-builder/BytecodeGraphBuilderForOf
* mjsunit/es6/spread-array

BUG=v8:5388
==========

to

==========
Implement JSBuiltinReducer for String.prototype[Symbol.iterator]

Known issues: these tests fail:
* cctest/test-run-bytecode-graph-builder/BytecodeGraphBuilderForOf
* mjsunit/es6/spread-array

BUG=v8:5388
==========

[jwolfe, 2016-10-17 21:40:07]

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

[jwolfe, 2016-10-17 21:40:23]

PTAL

[caitp, 2016-10-17 21:54:28]

caitp@igalia.com changed reviewers:
+ caitp@igalia.com

[caitp, 2016-10-17 21:54:29]

drive-by:

https://codereview.chromium.org/2422383002/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (left):

https://codereview.chromium.org/2422383002/diff/1/src/bootstrapper.cc#oldcode...
src/bootstrapper.cc:1478: iterator->shared()->set_native(true);
I don't think we need to get rid of the set_native() call, I believe it's there
for a reason. This is done by the InstallFunction() helpers.

I __believe__ this emulates strict mode and prevents undefined/null receivers
from being converted to the global proxy (among other things), which is hit by
tests and probably causes some failures.

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

https://codereview.chromium.org/2422383002/diff/1/src/compiler/js-builtin-red...
src/compiler/js-builtin-reducer.cc:1037: context, effect);
I mentioned this in the meeting, but other uses in the codebase supply the
`context` value input twice, followed by the effect input.

Example in js-typed-lowering.cc's JSTypedLowering::ReduceJSConvertReceiver(Node*
node):

```
Node* native_context = effect = graph()->NewNode(
          javascript()->LoadContext(0, Context::NATIVE_CONTEXT_INDEX, true),
          context, context, effect);
receiver = effect = graph()->NewNode(
          javascript()->LoadContext(0, Context::GLOBAL_PROXY_INDEX, true),
          native_context, native_context, effect);
```

--- I'm not exactly sure why both copies of the node are supplied, but this is
something I did in my version of the patch (probably got the idea from here). It
might be worth messing with this a bit to see if it solves the problem.

[jwolfe, 2016-10-17 22:20:59]

> I don't think we need to get rid of the set_native() call...
It had no effect on tests, but sounds like a good change.

> supply the `context` value input twice, followed by the effect input.
That fixed the tests, but it makes no sense to me.

How do I know how many parameters I'm supposed to pass to graph()->NewNode()? I
thought I could check the Operator() constructor parameters and sum up the
`*_in` values, but that leads me to think there should be only 2 parameters, not
3.

And how did this mistake get passed Verifier::VerifyNode()'s
`CHECK_EQ(OperatorProperties::GetTotalInputCount(node->op()),
node->InputCount());`?

[jwolfe, 2016-10-17 22:21:18]

Description was changed from

==========
Implement JSBuiltinReducer for String.prototype[Symbol.iterator]

Known issues: these tests fail:
* cctest/test-run-bytecode-graph-builder/BytecodeGraphBuilderForOf
* mjsunit/es6/spread-array

BUG=v8:5388
==========

to

==========
Implement JSBuiltinReducer for String.prototype[Symbol.iterator]

BUG=v8:5388
==========

[caitp, 2016-10-17 22:26:55]

On 2016/10/17 22:20:59, jwolfe wrote:
> > I don't think we need to get rid of the set_native() call...
> It had no effect on tests, but sounds like a good change.
> 
> > supply the `context` value input twice, followed by the effect input.
> That fixed the tests, but it makes no sense to me.
> 
> How do I know how many parameters I'm supposed to pass to graph()->NewNode()?
I
> thought I could check the Operator() constructor parameters and sum up the
> `*_in` values, but that leads me to think there should be only 2 parameters,
not
> 3.
> 
> And how did this mistake get passed Verifier::VerifyNode()'s
> `CHECK_EQ(OperatorProperties::GetTotalInputCount(node->op()),
> node->InputCount());`?

I'll defer to compiler folks for answering that one (I would love to know more
about how all of this is meant to work, too).

[caitp, 2016-10-17 23:45:44]

On 2016/10/17 22:26:55, caitp wrote:
> On 2016/10/17 22:20:59, jwolfe wrote:
> > > I don't think we need to get rid of the set_native() call...
> > It had no effect on tests, but sounds like a good change.
> > 
> > > supply the `context` value input twice, followed by the effect input.
> > That fixed the tests, but it makes no sense to me.
> > 
> > How do I know how many parameters I'm supposed to pass to
graph()->NewNode()?
> I
> > thought I could check the Operator() constructor parameters and sum up the
> > `*_in` values, but that leads me to think there should be only 2 parameters,
> not
> > 3.
> > 
> > And how did this mistake get passed Verifier::VerifyNode()'s
> > `CHECK_EQ(OperatorProperties::GetTotalInputCount(node->op()),
> > node->InputCount());`?
> 
> I'll defer to compiler folks for answering that one (I would love to know more
> about how all of this is meant to work, too).

Okay, after looking at it again, it makes perfect sense:

The second copy of the value input is the context input, which all JS operators
require (based on bool OperatorProperties::HasContextInput(const Operator* op)
which returns `IrOpcode::IsJSOpcode(op->opcode())`, which is true for
LoadContext(). So, I suppose all js-operator.cc items have an implicit context
input that isn't included in the operator definitions.

I'm not sure about the CHECK not failing, though. that's fishy and possibly a
bug.

[Benedikt Meurer, 2016-10-18 03:55:42]

Description was changed from

==========
Implement JSBuiltinReducer for String.prototype[Symbol.iterator]

BUG=v8:5388
==========

to

==========
[turbofan] Implement JSBuiltinReducer for String.prototype[Symbol.iterator].

BUG=v8:5388
==========

[Benedikt Meurer, 2016-10-18 03:58:51]

Nice, thanks. LGTM.

[jwolfe, 2016-10-18 16:05:15]

> I'm not sure about the CHECK not failing, though. that's fishy and possibly a
> bug.

Mystery solved. The test failures I originally got were exactly that CHECK
failing.

[jwolfe, 2016-10-18 16:05:47]

The CQ bit was checked by jwolfe@igalia.com

[commit-bot: I haz the power, 2016-10-18 16:05:57]

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

[caitp, 2016-10-18 16:08:20]

On 2016/10/18 16:05:15, jwolfe wrote:
> > I'm not sure about the CHECK not failing, though. that's fishy and possibly
a
> > bug.
> 
> Mystery solved. The test failures I originally got were exactly that CHECK
> failing.

ah, bingo :)

[commit-bot: I haz the power, 2016-10-18 16:32:37]

Description was changed from

==========
[turbofan] Implement JSBuiltinReducer for String.prototype[Symbol.iterator].

BUG=v8:5388
==========

to

==========
[turbofan] Implement JSBuiltinReducer for String.prototype[Symbol.iterator].

BUG=v8:5388
==========

[commit-bot: I haz the power, 2016-10-18 16:32:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-18 16:33:12]

Description was changed from

==========
[turbofan] Implement JSBuiltinReducer for String.prototype[Symbol.iterator].

BUG=v8:5388
==========

to

==========
[turbofan] Implement JSBuiltinReducer for String.prototype[Symbol.iterator].

BUG=v8:5388

Committed: https://crrev.com/21fda06c05e26b9085227682495248532fbb8357
Cr-Commit-Position: refs/heads/master@{#40406}
==========

[commit-bot: I haz the power, 2016-10-18 16:33:13]

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