VM [KERNEL] Avoid emitting :expr_temp and capturing :iterator

VM: [Kernel] Avoid emitting :expr_temp and capturing :iterator

This allow us bypass allocation of Context for some sync*
function such as in Iteration.tree.syncstar benchmark.

BUG=
R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/3d78c064124b9b47a0eed172658da62b62df2229

[Dmitry Olshansky, 2017-03-24 17:00:37]

dmitryolsh@google.com changed reviewers:
+ kustermann@google.com, vegorov@chromium.org

[Vyacheslav Egorov (Google), 2017-03-27 14:31:45]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2017-03-27 14:31:46]

LGTM with comments addressed.

https://codereview.chromium.org/2772963003/diff/1/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2772963003/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:46: bool NeedTemp() const { return need_expr_temp_; }
nit: style guide suggests naming simple getters like this:

bool needs_expr_temp() const {  ... }

https://codereview.chromium.org/2772963003/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:49: bool need_expr_temp_;
better name is needs_expr_temp_.

https://codereview.chromium.org/2772963003/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:53: bool NeedExprTemp(TreeNode* node) {
static bool NeedsExprTemp(...)

https://codereview.chromium.org/2772963003/diff/1/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/2772963003/diff/1/runtime/vm/scopes.cc#newcod...
runtime/vm/scopes.cc:643: (variable->name().raw() ==
Symbols::IteratorParameter().raw())) {
Can we move this to kernel_to_il.cc? We prefer to keep all scope related Kernel
specific changes confined to it.

Solution would be to find Symbols::IteratorParameter() named paramter and
set_is_forced_stack() it.

[Dmitry Olshansky, 2017-03-27 17:03:21]

Addressed comments

[Vyacheslav Egorov (Google), 2017-03-27 17:19:28]

Description was changed from

==========
VM [KERNEL] Avoid emitting :expr_temp and capturing :iterator

This allow us bypass allocation of Context for some sync*
function such as in Iteration.tree.syncstar benchmark.

BUG=
==========

to

==========
VM: [Kernel] Avoid emitting :expr_temp and capturing :iterator

This allow us bypass allocation of Context for some sync*
function such as in Iteration.tree.syncstar benchmark.

BUG=
==========

[Vyacheslav Egorov (Google), 2017-03-27 17:19:38]

Still LGTM

[Dmitry Olshansky, 2017-03-27 17:23:04]

Description was changed from

==========
VM: [Kernel] Avoid emitting :expr_temp and capturing :iterator

This allow us bypass allocation of Context for some sync*
function such as in Iteration.tree.syncstar benchmark.

BUG=
==========

to

==========
VM: [Kernel] Avoid emitting :expr_temp and capturing :iterator

This allow us bypass allocation of Context for some sync*
function such as in Iteration.tree.syncstar benchmark.

BUG=
R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/3d78c064124b9b47a0eed172658da62b62df2229
==========

[Dmitry Olshansky, 2017-03-27 17:23:09]

Committed patchset #2 (id:20001) manually as
3d78c064124b9b47a0eed172658da62b62df2229 (presubmit successful).

[Kevin Millikin (Google), 2017-03-28 07:25:38]

On 2017/03/27 17:23:09, Dmitry Olshansky wrote:
> Committed patchset #2 (id:20001) manually as
> 3d78c064124b9b47a0eed172658da62b62df2229 (presubmit successful).

This approach does not look good to me.

We should be able to avoid allocating an unneeded variable without using a new
compiler pass.  We're working on getting rid of the one unnecessary pass we
already have (ScopeBuilder) and adding another doesn't really help.

[jensj, 2017-03-28 07:31:05]

jensj@google.com changed reviewers:
+ jensj@google.com

[jensj, 2017-03-28 07:31:05]

Also I had to revert it @ https://codereview.chromium.org/2777413002