[turbofan] escape analysis supports arguments object and rest elements

[turbofan] escape analysis supports arguments object and rest elements

The new NewUnmappedArgumentsElements node now takes two inputs:
- the frame holding the arguments (current frame or arguments adaptor frame)
- the length of the suffix of passed arguments to be copied into the backing store

These inputs are computed with two new node types: 
ArgumentsFrame() 
ArgumentsLength[formal_parameter_count,is_rest_length](Node* arguments_frame)
The node type NewRestParameterElements can now be expressed with NewUnmappedArgumentsElements and an appropriate length and is thus not needed anymore.

In escape analysis, we lower loads from the length field of NewUnmappedArgumentsElements with its length input and if we find out that no write access to the arguments elements exists, we replace element loads with direct stack access and replace the NewUnmappedArgumentsElements node with a node of the new node type ArgumentsElementsState. This corresponds to an ObjectState node and gets translated into a deoptimizer instruction to allocate the backing store. Together with the already existing deoptimizer support for the actual arguments object/rest parameters, this allows to remove all allocations for arguments objects/rest parameters in this case. 
In the deoptimizer, we read the actual parameters from the stack while transforming the static deopt info into TranslatedValue objects.

If escape analysis cannot remove the backing store allocation, NewUnmappedArgumentsElements gets lo


BUG=v8:5726
Review-Url: https://codereview.chromium.org/2692753004
Cr-Commit-Position: refs/heads/master@{#43475}
Committed: https://chromium.googlesource.com/v8/v8/+/7467f16d737be89d9cac573a07026e477cff85db

[Tobias Tebbi, 2017-02-15 14:16:05]

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

[Tobias Tebbi, 2017-02-15 14:16:05]

https://codereview.chromium.org/2692753004/diff/60001/src/compiler/escape-ana...
File src/compiler/escape-analysis-reducer.h (right):

https://codereview.chromium.org/2692753004/diff/60001/src/compiler/escape-ana...
src/compiler/escape-analysis-reducer.h:63: std::set<Node*> arguments_elements_;
This is a small set, at most 2 elements, and only constructed once per
compilation. Is this OK, or does it have to be zone memory?

https://codereview.chromium.org/2692753004/diff/60001/src/compiler/escape-ana...
File src/compiler/escape-analysis.cc (right):

https://codereview.chromium.org/2692753004/diff/60001/src/compiler/escape-ana...
src/compiler/escape-analysis.cc:692: from =
object_analysis_->ResolveReplacement(from);
This is a drive-by fix. It was sound before, but imprecise.

[Tobias Tebbi, 2017-02-15 14:17:52]

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

[commit-bot: I haz the power, 2017-02-15 14:18:02]

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, 2017-02-15 14:22:52]

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

[commit-bot: I haz the power, 2017-02-15 14:22:53]

Dry run: Try jobs failed on following builders:
  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...)

[Benedikt Meurer, 2017-02-15 14:45:01]

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

[Benedikt Meurer, 2017-02-15 14:45:01]

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

[Benedikt Meurer, 2017-02-15 14:45:02]

LGTM. Adding jarin@ as 2nd reviewer.

[Jarin, 2017-02-15 16:17:15]

I have only looked at the deoptimizer so far, and have some questions there.

Overall, I think this needs much better explanation in the CL and some
explaining comments around the tricky parts of the code and the interfaces. I
have really hard time to wrap my head around the escape analysis part without
any explanations.

For example, the 'skip' argument needs explanation about what it means (for both
the operator and the translation).

https://codereview.chromium.org/2692753004/diff/100001/src/compiler/effect-co...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2692753004/diff/100001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.cc:1863: Node* frame =
graph()->NewNode(machine()->LoadFramePointer());
Please do not create nodes directly here and below. Instead, add an indirection
through GraphAssembler.

https://codereview.chromium.org/2692753004/diff/100001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/2692753004/diff/100001/src/deoptimizer.cc#new...
src/deoptimizer.cc:3369: length = std::max(0, length - skip);
This needs explanation.

https://codereview.chromium.org/2692753004/diff/100001/src/deoptimizer.cc#new...
src/deoptimizer.cc:3374: frame_index,
Missing value_index.

https://codereview.chromium.org/2692753004/diff/100001/src/deoptimizer.cc#new...
src/deoptimizer.cc:3394: // Thus we build a temporary structure in malloced
space.
Add a line that explains what is the return value.

https://codereview.chromium.org/2692753004/diff/100001/src/deoptimizer.cc#new...
src/deoptimizer.cc:3463: return translated_value.GetChildrenCount();
The amount of boilerplate here and below is unfortunate, I am still thinking
that the approach with materializing manually later would not lead to this
inconsistent code, where all the translations are dumb, but one is actually
tricky. In any case, this needs some serious explaining why the boilerplate is
necessary.

https://codereview.chromium.org/2692753004/diff/100001/src/deoptimizer.cc#new...
src/deoptimizer.cc:3750: int children_count = CreateNextTranslatedValue(
children_count -> nested_count (it is not children count anymore).

https://codereview.chromium.org/2692753004/diff/100001/src/deoptimizer.cc#new...
src/deoptimizer.cc:3752: input_frame_pointer, trace_file);
Why is the frame pointer passed twice here?

https://codereview.chromium.org/2692753004/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2692753004/diff/100001/src/objects.cc#newcode...
src/objects.cc:14361: os << "{arguments elements}";
Should not this also print the 'skip' argument? Could you make sure that
--print-code-verbose works after your changes?

[Tobias Tebbi, 2017-02-20 12:44:47]

Description was changed from

==========
[turbofan] escape analysis supports arguments object and rest elements

BUG=v8:5726
==========

to

==========
[turbofan] escape analysis supports arguments object and rest elements

This CL implements direct stack access, proper escape analysis and
deoptimization for  top-level (that is, not from inlined functions) unmapped
arguments objects and rest elements.

We extend NewUnmappedArgumentsElements to take two inputs:
- the frame holding the arguments (current frame or arguments adaptor frame)
- the length of the suffix of passed arguments to be copied into the backing
store

These inputs are computed with two new node types: ArgumentsFrame and
ArgumentsLength. The node type NewRestParameterElements can now be expressed
with NewUnmappedArgumentsElements and an appropriate length and is thus not
needed anymore.

In escape analysis, if we find out that no write access to the arguments
elements exists, we replace loads with direct stack access and replace the
elements backing store with a node of the new node type ArgumentsElementsState.
This gets translated into a deoptimizer instruction to allocate the backing
store. Together with the already existing deoptimizer support for the actual
arguments object/rest parameters, this allows to remove all allocations for
arguments objects/rest parameters in this case.

In the deoptimizer, we read the actual parameters from the stack while
transforming the static deopt info into TranslatedValue objects.


BUG=v8:5726
==========

[Tobias Tebbi, 2017-02-21 12:14:13]

Description was changed from

==========
[turbofan] escape analysis supports arguments object and rest elements

This CL implements direct stack access, proper escape analysis and
deoptimization for  top-level (that is, not from inlined functions) unmapped
arguments objects and rest elements.

We extend NewUnmappedArgumentsElements to take two inputs:
- the frame holding the arguments (current frame or arguments adaptor frame)
- the length of the suffix of passed arguments to be copied into the backing
store

These inputs are computed with two new node types: ArgumentsFrame and
ArgumentsLength. The node type NewRestParameterElements can now be expressed
with NewUnmappedArgumentsElements and an appropriate length and is thus not
needed anymore.

In escape analysis, if we find out that no write access to the arguments
elements exists, we replace loads with direct stack access and replace the
elements backing store with a node of the new node type ArgumentsElementsState.
This gets translated into a deoptimizer instruction to allocate the backing
store. Together with the already existing deoptimizer support for the actual
arguments object/rest parameters, this allows to remove all allocations for
arguments objects/rest parameters in this case.

In the deoptimizer, we read the actual parameters from the stack while
transforming the static deopt info into TranslatedValue objects.


BUG=v8:5726
==========

to

==========
[turbofan] escape analysis supports arguments object and rest elements

The new NewUnmappedArgumentsElements node now takes two inputs:
- the frame holding the arguments (current frame or arguments adaptor frame)
- the length of the suffix of passed arguments to be copied into the backing
store

These inputs are computed with two new node types: 
ArgumentsFrame() 
ArgumentsLength[formal_parameter_count,is_rest_length](Node* arguments_frame)
The node type NewRestParameterElements can now be expressed with
NewUnmappedArgumentsElements and an appropriate length and is thus not needed
anymore.

In escape analysis, we lower loads from the length field of
NewUnmappedArgumentsElements with its length input and if we find out that no
write access to the arguments elements exists, we replace element loads with
direct stack access and replace the NewUnmappedArgumentsElements node with a
node of the new node type ArgumentsElementsState. This corresponds to an
ObjectState node and gets translated into a deoptimizer instruction to allocate
the backing store. Together with the already existing deoptimizer support for
the actual arguments object/rest parameters, this allows to remove all
allocations for arguments objects/rest parameters in this case. 
In the deoptimizer, we read the actual parameters from the stack while
transforming the static deopt info into TranslatedValue objects.

If escape analysis cannot remove the backing store allocation,
NewUnmappedArgumentsElements gets lo


BUG=v8:5726
==========

[Tobias Tebbi, 2017-02-27 12:08:06]

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

[commit-bot: I haz the power, 2017-02-27 12:08:16]

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, 2017-02-27 12:11:46]

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

[commit-bot: I haz the power, 2017-02-27 12:11:47]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/21664)

[Jarin, 2017-02-27 13:24:34]

lgtm once the comments are addressed.

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/common-op...
File src/compiler/common-operator.cc (right):

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/common-op...
src/compiler/common-operator.cc:1242: int IsRestOf(Operator const* op) {
should be bool.

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/common-op...
File src/compiler/common-operator.h (right):

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/common-op...
src/compiler/common-operator.h:300: int IsRestOf(Operator const*);
Should be bool.

Still, some comment explaining what this means would be useful.

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/effect-co...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.cc:1859: if (is_rest_length) {
Please, briefly describe what this is doing in comments (one comment in the
beginning of each branch).

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/escape-an...
File src/compiler/escape-analysis-reducer.cc (right):

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/escape-an...
src/compiler/escape-analysis-reducer.cc:93: // Fallthrough.
Could we get rid of the fallthrough?

Case fallthroughs are evil.

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/escape-an...
src/compiler/escape-analysis-reducer.cc:433: // Fallthrough.
Could not you say here 
if (...) {
  load.push_back(use);
} else {
  escaping_use = true;
}
break;

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/escape-an...
src/compiler/escape-analysis-reducer.cc:435: if (!use->use_edges().empty()) {
Explain in a comment what this check does, please.

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/instructi...
File src/compiler/instruction.h (right):

https://codereview.chromium.org/2692753004/diff/120001/src/compiler/instructi...
src/compiler/instruction.h:1249: void PushArgumentsElements(int skip) {
How about 

int skip ==> bool is_rest

?

https://codereview.chromium.org/2692753004/diff/120001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/2692753004/diff/120001/src/deoptimizer.cc#new...
src/deoptimizer.cc:3350: // created on-the-fly based on dynamic information in
the optimized frame.
Awesome, thanks for the comments here and below!

https://codereview.chromium.org/2692753004/diff/120001/src/deoptimizer.cc#new...
src/deoptimizer.cc:3686: TranslatedValue translated_value =
This is unreachable code. Just say "return 0;" here.

[Tobias Tebbi, 2017-02-27 14:00:25]

Patchset #10 (id:180001) has been deleted

[Tobias Tebbi, 2017-02-27 14:07:26]

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

[commit-bot: I haz the power, 2017-02-27 14:07: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, 2017-02-27 14:13:44]

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

[commit-bot: I haz the power, 2017-02-27 14:13:45]

Dry run: Try jobs failed on following builders:
  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...)

[Tobias Tebbi, 2017-02-27 15:14:51]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2017-02-27 15:14:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2692753004/#ps260001
(title: "fixed typos")

[commit-bot: I haz the power, 2017-02-27 15:15:02]

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, 2017-02-27 15:19:32]

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

[commit-bot: I haz the power, 2017-02-27 15:19:33]

Try jobs failed on following builders:
  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...)

[Tobias Tebbi, 2017-02-27 15:23:38]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2017-02-27 15:23:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2692753004/#ps280001
(title: "removed dead code")

[commit-bot: I haz the power, 2017-02-27 15:23:42]

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, 2017-02-27 15:44:40]

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

[commit-bot: I haz the power, 2017-02-27 15:44:41]

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

[Tobias Tebbi, 2017-02-28 10:50:14]

The CQ bit was checked by tebbi@chromium.org

[Tobias Tebbi, 2017-02-28 10:50:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2692753004/#ps300001
(title: "handle the case where Deoptimizer::function_ is a Smi")

[commit-bot: I haz the power, 2017-02-28 10:50:24]

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, 2017-02-28 11:16:28]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1488279014067600,
"parent_rev": "2585ee06efb1b3051903451272959d083397295a", "commit_rev":
"7467f16d737be89d9cac573a07026e477cff85db"}

[commit-bot: I haz the power, 2017-02-28 11:16:33]

Description was changed from

==========
[turbofan] escape analysis supports arguments object and rest elements

The new NewUnmappedArgumentsElements node now takes two inputs:
- the frame holding the arguments (current frame or arguments adaptor frame)
- the length of the suffix of passed arguments to be copied into the backing
store

These inputs are computed with two new node types: 
ArgumentsFrame() 
ArgumentsLength[formal_parameter_count,is_rest_length](Node* arguments_frame)
The node type NewRestParameterElements can now be expressed with
NewUnmappedArgumentsElements and an appropriate length and is thus not needed
anymore.

In escape analysis, we lower loads from the length field of
NewUnmappedArgumentsElements with its length input and if we find out that no
write access to the arguments elements exists, we replace element loads with
direct stack access and replace the NewUnmappedArgumentsElements node with a
node of the new node type ArgumentsElementsState. This corresponds to an
ObjectState node and gets translated into a deoptimizer instruction to allocate
the backing store. Together with the already existing deoptimizer support for
the actual arguments object/rest parameters, this allows to remove all
allocations for arguments objects/rest parameters in this case. 
In the deoptimizer, we read the actual parameters from the stack while
transforming the static deopt info into TranslatedValue objects.

If escape analysis cannot remove the backing store allocation,
NewUnmappedArgumentsElements gets lo


BUG=v8:5726
==========

to

==========
[turbofan] escape analysis supports arguments object and rest elements

The new NewUnmappedArgumentsElements node now takes two inputs:
- the frame holding the arguments (current frame or arguments adaptor frame)
- the length of the suffix of passed arguments to be copied into the backing
store

These inputs are computed with two new node types: 
ArgumentsFrame() 
ArgumentsLength[formal_parameter_count,is_rest_length](Node* arguments_frame)
The node type NewRestParameterElements can now be expressed with
NewUnmappedArgumentsElements and an appropriate length and is thus not needed
anymore.

In escape analysis, we lower loads from the length field of
NewUnmappedArgumentsElements with its length input and if we find out that no
write access to the arguments elements exists, we replace element loads with
direct stack access and replace the NewUnmappedArgumentsElements node with a
node of the new node type ArgumentsElementsState. This corresponds to an
ObjectState node and gets translated into a deoptimizer instruction to allocate
the backing store. Together with the already existing deoptimizer support for
the actual arguments object/rest parameters, this allows to remove all
allocations for arguments objects/rest parameters in this case. 
In the deoptimizer, we read the actual parameters from the stack while
transforming the static deopt info into TranslatedValue objects.

If escape analysis cannot remove the backing store allocation,
NewUnmappedArgumentsElements gets lo


BUG=v8:5726

Review-Url: https://codereview.chromium.org/2692753004
Cr-Commit-Position: refs/heads/master@{#43475}
Committed:
https://chromium.googlesource.com/v8/v8/+/7467f16d737be89d9cac573a07026e477cf...
==========

[commit-bot: I haz the power, 2017-02-28 11:16:34]

Committed patchset #15 (id:300001) as
https://chromium.googlesource.com/v8/v8/+/7467f16d737be89d9cac573a07026e477cf...

[Michael Achenbach, 2017-02-28 14:06:49]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2017-02-28 14:06:50]

Looks like we got slightly less predictable since this commit:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20predictable/...

Any idea? +ishell

[Michael Achenbach, 2017-03-03 13:29:36]

On 2017/02/28 14:06:50, Michael Achenbach wrote:
> Looks like we got slightly less predictable since this commit:
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20predictable/...
> 
> Any idea? +ishell

Not sure who did what, but it looks somehow green again:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20predictable?...

[Tobias Tebbi, 2017-03-03 14:02:01]

On 2017/03/03 13:29:36, Michael Achenbach wrote:
> On 2017/02/28 14:06:50, Michael Achenbach wrote:
> > Looks like we got slightly less predictable since this commit:
> >
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20predictable/...
> > 
> > Any idea? +ishell
> 
> Not sure who did what, but it looks somehow green again:
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20predictable?...

These two CLs fix the issue and the underlying problem:
https://codereview.chromium.org/2728983002/
https://codereview.chromium.org/2730753003/

One is not landed yet, but probably it vanished in the same way it appeared:
another Turbofan opcode made hash collisions sufficiently improbable again.