[turbofan] Add an extra frame state for deoptimization before binary op.

[turbofan] Add an extra frame state for deoptimization before binary op.

BUG=
R=bmeurer@chromium.org

Committed: https://crrev.com/02e318517f74ccca3545369e4b93d56f599c1a7a
Cr-Commit-Position: refs/heads/master@{#27060}

[Jarin, 2015-03-06 16:16:02]

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

[Jarin, 2015-03-06 16:16:20]

Could you take a look, please?

[Benedikt Meurer, 2015-03-09 05:32:35]

General direction looks good.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2975:
DCHECK_EQ(OperatorProperties::GetFrameStateInputCount(node->op()), 1);
Nit: DCHECK_XX(expected, actual)

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2977:
DCHECK_EQ(NodeProperties::GetFrameStateInput(node, 0)->opcode(),
Nit: DCHECK_XX(expected, actual)

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2989:
DCHECK_EQ(OperatorProperties::GetFrameStateInputCount(node->op()), 2);
Nit: DCHECK_XX(expected, actual)

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2991:
DCHECK_EQ(NodeProperties::GetFrameStateInput(node, 0)->opcode(),
Nit: DCHECK_XX(expected, actual)

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2996:
DCHECK_EQ(NodeProperties::GetFrameStateInput(node, 1)->opcode(),
Nit: DCHECK_XX(expected, actual)

https://codereview.chromium.org/983153002/diff/20001/src/compiler/change-lowe...
File src/compiler/change-lowering.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:232: DCHECK(FLAG_turbo_deoptimization ==
DCHECK_EQ?

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-generic-...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-generic-...
src/compiler/js-generic-lowering.cc:213: for (int i = new_frame_state_count; i <
old_frame_state_count; i++) {
This loop doesn't look right. If you remove input i, then input i+1 becomes
input i, and so on. So you're probably removing non-frame-state inputs here.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:379:
DCHECK(OperatorProperties::GetFrameStateInputCount(node->op()) == 1);
DCHECK_EQ?

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-typed-lo...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-typed-lo...
src/compiler/js-typed-lowering.cc:650:
DCHECK(OperatorProperties::GetFrameStateInputCount(node->op()) == 1);
DCHECK_EQ?

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-typed-lo...
src/compiler/js-typed-lowering.cc:773: DCHECK(FLAG_turbo_deoptimization ==
DCHECK_EQ?

https://codereview.chromium.org/983153002/diff/20001/src/compiler/node-proper...
File src/compiler/node-properties.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/node-proper...
src/compiler/node-properties.cc:62: DCHECK(index <
OperatorProperties::GetFrameStateInputCount(node->op()));
DCHECK_LT

https://codereview.chromium.org/983153002/diff/20001/src/compiler/node-proper...
src/compiler/node-properties.cc:143: DCHECK(index <
OperatorProperties::GetFrameStateInputCount(node->op()));
DCHECK_LT

https://codereview.chromium.org/983153002/diff/20001/src/compiler/operator-pr...
File src/compiler/operator-properties.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/operator-pr...
src/compiler/operator-properties.cc:23: int
OperatorProperties::GetFrameStateInputCount(const Operator* op) {
I think this method now deserves a unit test.

https://codereview.chromium.org/983153002/diff/20001/test/cctest/compiler/sim...
File test/cctest/compiler/simplified-graph-builder.cc (right):

https://codereview.chromium.org/983153002/diff/20001/test/cctest/compiler/sim...
test/cctest/compiler/simplified-graph-builder.cc:51:
DCHECK(OperatorProperties::GetFrameStateInputCount(op) == 0);
DCHECK_EQ

[Jarin, 2015-03-09 07:44:56]

Patchset #3 (id:40001) has been deleted

[Jarin, 2015-03-09 07:47:12]

All addressed now. Thanks!

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2975:
DCHECK_EQ(OperatorProperties::GetFrameStateInputCount(node->op()), 1);
On 2015/03/09 05:32:34, Benedikt Meurer wrote:
> Nit: DCHECK_XX(expected, actual)

Done.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2977:
DCHECK_EQ(NodeProperties::GetFrameStateInput(node, 0)->opcode(),
On 2015/03/09 05:32:34, Benedikt Meurer wrote:
> Nit: DCHECK_XX(expected, actual)

Done.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2989:
DCHECK_EQ(OperatorProperties::GetFrameStateInputCount(node->op()), 2);
On 2015/03/09 05:32:34, Benedikt Meurer wrote:
> Nit: DCHECK_XX(expected, actual)

Done.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2991:
DCHECK_EQ(NodeProperties::GetFrameStateInput(node, 0)->opcode(),
On 2015/03/09 05:32:34, Benedikt Meurer wrote:
> Nit: DCHECK_XX(expected, actual)

Done.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2996:
DCHECK_EQ(NodeProperties::GetFrameStateInput(node, 1)->opcode(),
On 2015/03/09 05:32:34, Benedikt Meurer wrote:
> Nit: DCHECK_XX(expected, actual)

Done.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/change-lowe...
File src/compiler/change-lowering.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/change-lowe...
src/compiler/change-lowering.cc:232: DCHECK(FLAG_turbo_deoptimization ==
On 2015/03/09 05:32:34, Benedikt Meurer wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-generic-...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-generic-...
src/compiler/js-generic-lowering.cc:213: for (int i = new_frame_state_count; i <
old_frame_state_count; i++) {
On 2015/03/09 05:32:34, Benedikt Meurer wrote:
> This loop doesn't look right. If you remove input i, then input i+1 becomes
> input i, and so on. So you're probably removing non-frame-state inputs here.

Good catch! Done.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:379:
DCHECK(OperatorProperties::GetFrameStateInputCount(node->op()) == 1);
On 2015/03/09 05:32:34, Benedikt Meurer wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-typed-lo...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/js-typed-lo...
src/compiler/js-typed-lowering.cc:650:
DCHECK(OperatorProperties::GetFrameStateInputCount(node->op()) == 1);
On 2015/03/09 05:32:34, Benedikt Meurer wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/983153002/diff/20001/src/compiler/operator-pr...
File src/compiler/operator-properties.cc (right):

https://codereview.chromium.org/983153002/diff/20001/src/compiler/operator-pr...
src/compiler/operator-properties.cc:23: int
OperatorProperties::GetFrameStateInputCount(const Operator* op) {
On 2015/03/09 05:32:35, Benedikt Meurer wrote:
> I think this method now deserves a unit test.

It already has one.

https://codereview.chromium.org/983153002/diff/20001/test/cctest/compiler/sim...
File test/cctest/compiler/simplified-graph-builder.cc (right):

https://codereview.chromium.org/983153002/diff/20001/test/cctest/compiler/sim...
test/cctest/compiler/simplified-graph-builder.cc:51:
DCHECK(OperatorProperties::GetFrameStateInputCount(op) == 0);
On 2015/03/09 05:32:35, Benedikt Meurer wrote:
> DCHECK_EQ

Done.

[Benedikt Meurer, 2015-03-09 07:57:23]

lgtm

[Jarin, 2015-03-09 08:04:56]

The CQ bit was checked by jarin@chromium.org

[commit-bot: I haz the power, 2015-03-09 08:05:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/983153002/60001

[commit-bot: I haz the power, 2015-03-09 08:12:47]

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

[commit-bot: I haz the power, 2015-03-09 08:12:48]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-03-09 08:12:49]

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

[Jarin, 2015-03-09 08:37:25]

Committed patchset #3 (id:60001) manually as
02e318517f74ccca3545369e4b93d56f599c1a7a (presubmit successful).

[commit-bot: I haz the power, 2015-03-09 08:37:32]

Patchset 3 (id:??) landed as
https://crrev.com/02e318517f74ccca3545369e4b93d56f599c1a7a
Cr-Commit-Position: refs/heads/master@{#27060}