[turbofan] Generate loop exits in the bytecode graph builder.

[turbofan] Generate loop exits in the bytecode graph builder.

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

[Jarin, 2016-07-28 17:39:52]

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

[commit-bot: I haz the power, 2016-07-28 17:40:02]

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

[Jarin, 2016-07-28 17:44:04]

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

[commit-bot: I haz the power, 2016-07-28 17:44: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-07-28 18:02:13]

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

[commit-bot: I haz the power, 2016-07-28 18:02:14]

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

[Jarin, 2016-07-29 05:53:02]

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

[commit-bot: I haz the power, 2016-07-29 05:53:10]

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

[Jarin, 2016-07-29 05:58:28]

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

[Jarin, 2016-07-29 05:58:29]

Could you take a look, please?

[commit-bot: I haz the power, 2016-07-29 06:14:58]

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

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

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-07-29 08:54:46]

LGTM. Just nits. I am unfamiliar with the change in the code generator, I'll
leave reviewing that to Benedikt.

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:470: void
BytecodeGraphBuilder::Environment::PrepareForLoopExit(Node* loop) {
nit: Can we move this up a few functions close to "PerpareForLoop" and
"PrepareForOsr"?

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:482:
graph()->NewNode(common()->LoopExitValue(), (*values())[i], loop_exit);
nit: Just "values_[i]".

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:483: (*values())[i] = rename;
nit: Just "values_[i]".

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:486: // Rename the effect.
nit: Can we handle the effect before the values, just to keep it in sync with
other methods?

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:152: // {target_offset}.
nit: s/{origin_offset}/the current bytecode offset/

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
File src/compiler/bytecode-loop-analysis.h (right):

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-loop-analysis.h:8: #include "src/bit-vector.h"
nit: Seems the BitVector is not needed in the header?

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-loop-analysis.h:52: ZoneMap<int, int> backedge_to_header_;
nit: Can we leave a comment here that both of these maps will contain one entry
per loop? That would clarify that they both take just O(n) space where n is the
number of loops. Also might we worth mentioning that both key and value
represent bytecode offsets.

[Michael Starzinger, 2016-07-29 08:58:21]

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:479: // Rename the environmnent values.
Is it OK not to handle "context_" here?

[Jarin, 2016-07-29 11:29:53]

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

[commit-bot: I haz the power, 2016-07-29 11:30:00]

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

[Jarin, 2016-07-29 11:36:47]

Thanks for the great review!

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:470: void
BytecodeGraphBuilder::Environment::PrepareForLoopExit(Node* loop) {
On 2016/07/29 08:54:46, Michael Starzinger wrote:
> nit: Can we move this up a few functions close to "PerpareForLoop" and
> "PrepareForOsr"?

Done.

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:479: // Rename the environmnent values.
On 2016/07/29 08:58:21, Michael Starzinger wrote:
> Is it OK not to handle "context_" here?

Not ok, great catch! (Well, not ok if we want to account for all loop exiting
values, which was my goal.)

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:482:
graph()->NewNode(common()->LoopExitValue(), (*values())[i], loop_exit);
On 2016/07/29 08:54:46, Michael Starzinger wrote:
> nit: Just "values_[i]".

Done.

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:483: (*values())[i] = rename;
On 2016/07/29 08:54:45, Michael Starzinger wrote:
> nit: Just "values_[i]".

Done.

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:486: // Rename the effect.
On 2016/07/29 08:54:46, Michael Starzinger wrote:
> nit: Can we handle the effect before the values, just to keep it in sync with
> other methods?

Done.

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:152: // {target_offset}.
On 2016/07/29 08:54:46, Michael Starzinger wrote:
> nit: s/{origin_offset}/the current bytecode offset/

Done.

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
File src/compiler/bytecode-loop-analysis.h (right):

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-loop-analysis.h:8: #include "src/bit-vector.h"
On 2016/07/29 08:54:46, Michael Starzinger wrote:
> nit: Seems the BitVector is not needed in the header?

Done.

https://codereview.chromium.org/2188533002/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-loop-analysis.h:52: ZoneMap<int, int> backedge_to_header_;
On 2016/07/29 08:54:46, Michael Starzinger wrote:
> nit: Can we leave a comment here that both of these maps will contain one
entry
> per loop? That would clarify that they both take just O(n) space where n is
the
> number of loops. Also might we worth mentioning that both key and value
> represent bytecode offsets.

Done.

[commit-bot: I haz the power, 2016-07-29 11:54:18]

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

[commit-bot: I haz the power, 2016-07-29 11:54:19]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-08-01 17:33:45]

I only did the code generator part as discussed. LGTM for that, once comment
addressed.

https://codereview.chromium.org/2188533002/diff/140001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2188533002/diff/140001/src/compiler/code-gene...
src/compiler/code-generator.cc:867: if (type.representation() ==
MachineRepresentation::kTagged) {
As discussed offline, this should only ever happen on 32-bit platforms. Please
add appropriate checking.

[Jarin, 2016-08-08 05:50:52]

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

[commit-bot: I haz the power, 2016-08-08 05:50:57]

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-08 06:07:41]

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

[commit-bot: I haz the power, 2016-08-08 06:07:43]

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

[Jarin, 2016-08-08 09:20:15]

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

[commit-bot: I haz the power, 2016-08-08 09:20:25]

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-08-08 09:22:06]

lgtm

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

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

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

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-08-08 09:59:02]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2016-08-08 09:59:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2188533002/#ps180001
(title: "Fix 64-bit")

[commit-bot: I haz the power, 2016-08-08 09:59:17]

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-08 10:01:17]

Committed patchset #10 (id:180001)

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

Description was changed from

==========
[turbofan] Generate loop exits in the bytecode graph builder.
==========

to

==========
[turbofan] Generate loop exits in the bytecode graph builder.

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

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

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