[interpreter] Use VisitForTest for loop conditions

[interpreter] Use VisitForTest for loop conditions

Changes the control flow builder classes to make use of the
BytecodeLabels helper class.

BUG=v8:4280
LOG=n

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

[klaasb, 2016-08-16 17:03:19]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 17:03:21]

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

[klaasb, 2016-08-16 19:24:08]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 19:24:12]

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-08-16 19:42:46]

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

[commit-bot: I haz the power, 2016-08-16 19:42:47]

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

[klaasb, 2016-08-17 10:40:30]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 10:40:34]

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-08-17 11:04:30]

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

[commit-bot: I haz the power, 2016-08-17 11:04:31]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-08-17 11:37:20]

klaasb@google.com changed reviewers:
+ mstarzinger@chromium.org, rmcilroy@chromium.org

[klaasb, 2016-08-17 11:37:21]

Have a look at the arm32-perf results (gsutil.upload) for a supposed 470%
improvement on Octane-Ignition-DeltaBlue.

[rmcilroy, 2016-08-17 16:20:43]

Looks great overall. One question about how this interacts with
bytecode-loop-analysis though.

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1179: VisitForTest(stmt->cond(),
loop_builder.header_labels(),
As discussed offline, this might break bytecode-loop-analysis.cc. Could you have
a look, and add a test in test-bytecode-graph-builder that stresses the case
where we emit multiple jumps to the loop header.

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-label.h (right):

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-label.h:67: bool is_bound() const { return
!labels_.empty() && labels_.at(0).is_bound(); }
Could you add a DCHECK that if one label is bound then all labels are bound.

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.cc (right):

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.cc:47: DCHECK(continue_labels_.empty() ||
continue_labels_.is_bound());
Also DCHECK header labels

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.h (right):

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.h:108: // Unbound labels that identify
jumps for continue statements in the code.
update comment to mention header labels.

[klaasb, 2016-08-18 11:40:00]

Fixed the bytecode-loop-analysis to allow multiple backedges for loops and added
test cases for that.

As discussed offline: This would still not work, if we had two loops with the
entry collapsing on the same bytecode offset, possibly because we wouldn't
generate OsrPoll and StackCheck. This will be caught by a DCHECK however and
currently there will always be a StackCheck bytecode for each loop.

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1179: VisitForTest(stmt->cond(),
loop_builder.header_labels(),
On 2016/08/17 16:20:43, rmcilroy wrote:
> As discussed offline, this might break bytecode-loop-analysis.cc. Could you
have
> a look, and add a test in test-bytecode-graph-builder that stresses the case
> where we emit multiple jumps to the loop header.

Done.

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-label.h (right):

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-label.h:67: bool is_bound() const { return
!labels_.empty() && labels_.at(0).is_bound(); }
On 2016/08/17 16:20:43, rmcilroy wrote:
> Could you add a DCHECK that if one label is bound then all labels are bound.

Done.

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.cc (right):

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.cc:47: DCHECK(continue_labels_.empty() ||
continue_labels_.is_bound());
On 2016/08/17 16:20:43, rmcilroy wrote:
> Also DCHECK header labels

Done.

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.h (right):

https://codereview.chromium.org/2254493002/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.h:108: // Unbound labels that identify
jumps for continue statements in the code.
On 2016/08/17 16:20:43, rmcilroy wrote:
> update comment to mention header labels.

Done.

[klaasb, 2016-08-18 11:43:21]

Description was changed from

==========
[interpreter] Use VisitForTest for loop conditions

Changes the control flow builder classes to make use of the
BytecodeLabels helper class.

BUG=v8:4280
LOG=n
==========

to

==========
[interpreter] Use VisitForTest for loop conditions

Changes the control flow builder classes to make use of the
BytecodeLabels helper class.

BUG=v8:4280
LOG=n
==========

[klaasb, 2016-08-18 11:43:21]

klaasb@google.com changed reviewers:
+ jarin@chromium.org

[klaasb, 2016-08-18 11:46:31]

klaasb@google.com changed reviewers:
- mstarzinger@chromium.org, rmcilroy@chromium.org

[klaasb, 2016-08-18 11:46:32]

Hi Jaroslav,

I modified the bytecode loop analysis to work with multiple backedges generated
when short-circuiting do-while conditions.

From looking and testing I'm convinced it works with the bytecode graph builder,
but please take a look.

[klaasb, 2016-08-18 11:46:55]

klaasb@google.com changed reviewers:
+ mstarzinger@chromium.org, rmcilroy@chromium.org

[klaasb, 2016-08-18 11:48:58]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 11:49:03]

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

[rmcilroy, 2016-08-18 12:33:22]

LGTM assuming Jaro and Michi are happy with the loop analysis changes.

[Jarin, 2016-08-18 12:49:51]

The loop analysis code looks good to me.

https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-loop-analysis.cc (right):

https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-loop-analysis.cc:42: // another loop entry or an outer
loop backedge.
Could you explain how you guard against the assumption being violated? (I have
convinced myself the DCHECK on line 60 somehow does the job, but it would be
good to have a coherent argument here.)

[Michael Starzinger, 2016-08-18 13:24:28]

LGTM modulo Jaro's comment. Note that I didn't do a though review and basically
only looked at the loop analysis and the high-level idea.

https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-loop-analysis.cc (right):

https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-loop-analysis.cc:20: zone_(zone),
nit: Could we add sensible initializations of both fields,
"found_current_backedge_" as well as "current_loop_offset_" here?

https://codereview.chromium.org/2254493002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2254493002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1179: VisitForTest(stmt->cond(),
loop_builder.header_labels(),
Just for posterity: I am fine with this. But this will make it harder to
potentially merge the {OsrPoll} instruction into the backwards jump instruction
(if we ever find we need to do that).

[klaasb, 2016-08-18 13:38:02]

Thanks for the comment, see below for a better correctness argument.

https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-loop-analysis.cc (right):

https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-loop-analysis.cc:42: // another loop entry or an outer
loop backedge.
On 2016/08/18 12:49:50, Jarin wrote:
> Could you explain how you guard against the assumption being violated? (I have
> convinced myself the DCHECK on line 60 somehow does the job, but it would be
> good to have a coherent argument here.)

The combination of the DCHECKs on 58, 60 and 63 does it.
When we encounter a backedge, we are either still searching and finding
backedges of the current loop (63), or we need to have found at least one back
edge to the current loop (58) and thus the backedge must target the parent loop
(60), which also guarantees that we found the (first) backedge of the parent.

Assuming there could be a backedge out of order with a backedge to the parent
loop, then 63 would fail, as target_offset > current_loop_offset_.

After setting the current_loop_offset_ at loop entry, we can only encounter a
backedge to the current loop (63) or parent loop (60) and 58 checks that if it
goes to the parent loop, we actually ended the current loop.

Only in the case of a sibling loop do we enter the branch here and then
resetting found_current_backedge to false guarantees failure for invalid
backedges at 58/63. I moved line 48 to the branch to have it at the actually
relevant place.

I've also expanded this comment to summarize the argument above.

[klaasb, 2016-08-18 13:50:11]

https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-loop-analysis.cc (right):

https://codereview.chromium.org/2254493002/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-loop-analysis.cc:20: zone_(zone),
On 2016/08/18 13:24:28, Michael Starzinger wrote:
> nit: Could we add sensible initializations of both fields,
> "found_current_backedge_" as well as "current_loop_offset_" here?

Done.

https://codereview.chromium.org/2254493002/diff/140001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2254493002/diff/140001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1179: VisitForTest(stmt->cond(),
loop_builder.header_labels(),
On 2016/08/18 13:24:28, Michael Starzinger wrote:
> Just for posterity: I am fine with this. But this will make it harder to
> potentially merge the {OsrPoll} instruction into the backwards jump
instruction
> (if we ever find we need to do that).

True. However, when/if that's needed we could just replace the header_labels()
here with new BytecodeLabels which could be bound at a place the backwards jump
happens.
We could instead refactor VisitForTest to use some builder for generating the
jumps.

[klaasb, 2016-08-18 14:07:26]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 14:07:32]

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-08-18 14:09:43]

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

[commit-bot: I haz the power, 2016-08-18 14:09:44]

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

[klaasb, 2016-08-18 14:16:31]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 14:16:45]

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-08-18 14:58:03]

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

[commit-bot: I haz the power, 2016-08-18 14:58:04]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-08-19 08:47:20]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-08-19 08:47:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2254493002/#ps200001
(title: "typo")

[commit-bot: I haz the power, 2016-08-19 08:47:31]

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-08-19 08:50:20]

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

[commit-bot: I haz the power, 2016-08-19 08:50:21]

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/22102)

[klaasb, 2016-08-19 08:56:12]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-08-19 08:56:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2254493002/#ps220001
(title: "rebase(line) golden file")

[commit-bot: I haz the power, 2016-08-19 08:56:23]

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-08-19 09:21:44]

Description was changed from

==========
[interpreter] Use VisitForTest for loop conditions

Changes the control flow builder classes to make use of the
BytecodeLabels helper class.

BUG=v8:4280
LOG=n
==========

to

==========
[interpreter] Use VisitForTest for loop conditions

Changes the control flow builder classes to make use of the
BytecodeLabels helper class.

BUG=v8:4280
LOG=n
==========

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

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-19 09:22:52]

Description was changed from

==========
[interpreter] Use VisitForTest for loop conditions

Changes the control flow builder classes to make use of the
BytecodeLabels helper class.

BUG=v8:4280
LOG=n
==========

to

==========
[interpreter] Use VisitForTest for loop conditions

Changes the control flow builder classes to make use of the
BytecodeLabels helper class.

BUG=v8:4280
LOG=n

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

[commit-bot: I haz the power, 2016-08-19 09:22:53]

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