[Interpreter] Add CreateClosure to BytecodeGraphBuilder.

[Interpreter] Add CreateClosure to BytecodeGraphBuilder.

Adds code and tests to support CreateClosure bytecode when building
graphs.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/4cceb11b0929abcbc82bf0854554a9b66003335d
Cr-Commit-Position: refs/heads/master@{#32224}

[oth, 2015-11-20 15:10:57]

oth@chromium.org changed reviewers:
+ bmeurer@chromium.org, mythria@chromium.org

[oth, 2015-11-20 15:10:57]

Benedikt, Mythrie: PTAL. Thanks!

[mythria, 2015-11-20 17:01:00]

lgtm

[Michael Starzinger, 2015-11-20 17:11:43]

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

[Michael Starzinger, 2015-11-20 17:11:43]

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:648:
OpParameter<Handle<SharedFunctionInfo>>(accumulator));
Not sure what this OpParameter call is trying to achieve, but I am not aware of
any nodes whose parameter is Handle<SharedFunctionInfo>. This won't work.

If the assumption is that the "accumulator" node is a HeapConstant, then please
use a HeapObjectMatcher to access it.

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/js-operato...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/js-operato...
src/compiler/js-operator.cc:387:
lhs.shared_info().is_identical_to(rhs.shared_info());
Please don't! The equality operator needs to be based on the same semantics as
the hashing operator. Since the target a handle location can point to can
relocate, which would change the hash, we decided to base both hashing and
equality on the location. This is on purpose.

https://codereview.chromium.org/1458603012/diff/60001/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/1458603012/diff/60001/src/handles.h#newcode79
src/handles.h:79: // Also note that Handles do not provide default equality
comparison or hashing
Please appreciate this comment and remove the equality operator. :)

[oth, 2015-11-21 14:19:47]

Thanks Michi, comments incorporated.

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:648:
OpParameter<Handle<SharedFunctionInfo>>(accumulator));
On 2015/11/20 17:11:43, Michael Starzinger wrote:
> Not sure what this OpParameter call is trying to achieve, but I am not aware
of
> any nodes whose parameter is Handle<SharedFunctionInfo>. This won't work.
> 
> If the assumption is that the "accumulator" node is a HeapConstant, then
please
> use a HeapObjectMatcher to access it.

A JSCreateClosure node taking a Handle<SharedFunctionInfo> argument is
constructed two lines below.

HeapObjectMatcher is a wrapper around OpParameter. One of these is widely used.

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/js-operato...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/js-operato...
src/compiler/js-operator.cc:387:
lhs.shared_info().is_identical_to(rhs.shared_info());
On 2015/11/20 17:11:43, Michael Starzinger wrote:
> Please don't! The equality operator needs to be based on the same semantics as
> the hashing operator. Since the target a handle location can point to can
> relocate, which would change the hash, we decided to base both hashing and
> equality on the location. This is on purpose.

Okay. I haved fixed the matcher here as the problem was comparing two
CreateClosureParameters instances. The two instances naturally have the same
SharedFunctionInfo objects, but different locations.

https://codereview.chromium.org/1458603012/diff/60001/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/1458603012/diff/60001/src/handles.h#newcode79
src/handles.h:79: // Also note that Handles do not provide default equality
comparison or hashing
On 2015/11/20 17:11:43, Michael Starzinger wrote:
> Please appreciate this comment and remove the equality operator. :)

Yes! This wasn't intended to be part of the CL. I was attempting to understand
why compilation was failing with MSVC, but without direct access to a machine.

[oth, 2015-11-21 14:19:47]

Thanks Michi, comments incorporated.

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:648:
OpParameter<Handle<SharedFunctionInfo>>(accumulator));
On 2015/11/20 17:11:43, Michael Starzinger wrote:
> Not sure what this OpParameter call is trying to achieve, but I am not aware
of
> any nodes whose parameter is Handle<SharedFunctionInfo>. This won't work.
> 
> If the assumption is that the "accumulator" node is a HeapConstant, then
please
> use a HeapObjectMatcher to access it.

A JSCreateClosure node taking a Handle<SharedFunctionInfo> argument is
constructed two lines below.

HeapObjectMatcher is a wrapper around OpParameter. One of these is widely used.

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/js-operato...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/1458603012/diff/60001/src/compiler/js-operato...
src/compiler/js-operator.cc:387:
lhs.shared_info().is_identical_to(rhs.shared_info());
On 2015/11/20 17:11:43, Michael Starzinger wrote:
> Please don't! The equality operator needs to be based on the same semantics as
> the hashing operator. Since the target a handle location can point to can
> relocate, which would change the hash, we decided to base both hashing and
> equality on the location. This is on purpose.

Okay. I haved fixed the matcher here as the problem was comparing two
CreateClosureParameters instances. The two instances naturally have the same
SharedFunctionInfo objects, but different locations.

https://codereview.chromium.org/1458603012/diff/60001/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/1458603012/diff/60001/src/handles.h#newcode79
src/handles.h:79: // Also note that Handles do not provide default equality
comparison or hashing
On 2015/11/20 17:11:43, Michael Starzinger wrote:
> Please appreciate this comment and remove the equality operator. :)

Yes! This wasn't intended to be part of the CL. I was attempting to understand
why compilation was failing with MSVC, but without direct access to a machine.

[Benedikt Meurer, 2015-11-22 19:00:53]

https://codereview.chromium.org/1458603012/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1458603012/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:648: HeapObjectMatcher
shared_info_matcher(accumulator);
Why do you need a HeapObjectMatcher at all here? The shared info should be a
literal for the CreateClosure bytecode and not an input.

[oth, 2015-11-22 21:43:59]

Thanks Benedikt, can you confirm I understood correctly?

https://codereview.chromium.org/1458603012/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1458603012/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:648: HeapObjectMatcher
shared_info_matcher(accumulator);
On 2015/11/22 19:00:53, Benedikt Meurer wrote:
> Why do you need a HeapObjectMatcher at all here? The shared info should be a
> literal for the CreateClosure bytecode and not an input.

Just to clarify, is question suggesting the CreateClosure bytecode
be refined to include the literal rather than having the previous
bytecode load the value from the constant pool?

Current:
LdaConstant <constant_pool_index>
CreateClosure <pretenure_flag>

New:
CreateClosure <constant_pool_index> <pretenture_flag>

[Benedikt Meurer, 2015-11-23 09:01:39]

On 2015/11/22 21:43:59, oth wrote:
> Thanks Benedikt, can you confirm I understood correctly?
> 
>
https://codereview.chromium.org/1458603012/diff/80001/src/compiler/bytecode-g...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/1458603012/diff/80001/src/compiler/bytecode-g...
> src/compiler/bytecode-graph-builder.cc:648: HeapObjectMatcher
> shared_info_matcher(accumulator);
> On 2015/11/22 19:00:53, Benedikt Meurer wrote:
> > Why do you need a HeapObjectMatcher at all here? The shared info should be a
> > literal for the CreateClosure bytecode and not an input.
> 
> Just to clarify, is question suggesting the CreateClosure bytecode
> be refined to include the literal rather than having the previous
> bytecode load the value from the constant pool?
> 
> Current:
> LdaConstant <constant_pool_index>
> CreateClosure <pretenure_flag>
> 
> New:
> CreateClosure <constant_pool_index> <pretenture_flag>

Yeah I think the bytecode should look like this, otherwise the graph builder
will essentially have to match sequences of byte codes instead of being built on
single bytecodes (which is essentially what you achieved by using the
HeapObjectMatcher there).

[Michael Starzinger, 2015-11-23 11:18:38]

https://codereview.chromium.org/1458603012/diff/100001/test/unittests/compile...
File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right):

https://codereview.chromium.org/1458603012/diff/100001/test/unittests/compile...
test/unittests/compiler/bytecode-graph-builder-unittest.cc:842:
Handle<SharedFunctionInfo> shared_info = inner_graph_and_shared_info.second;
If the only thing needed from the inner bytecode is a the "shared_info", then I
would vote for just using "isolate()->object_function()->shared()" instead.

https://codereview.chromium.org/1458603012/diff/100001/test/unittests/compile...
File test/unittests/compiler/node-test-utils.cc (right):

https://codereview.chromium.org/1458603012/diff/100001/test/unittests/compile...
test/unittests/compiler/node-test-utils.cc:1925: return
(PrintMatchAndExplain(&parameters, "value", parameters_matcher_,
Instead of going through extra lengths of making the "CreateClosureParameters"
comparable, can we instead just extract it from the node and compare the values
within? This would make the entire CreateClosureParametersMatcher obsolete. This
is similar to what we do in other matcher, IsJSLoadNamedMatcher for example.

[oth, 2015-11-24 12:29:36]

Benedikt: this change revises the CreateClosure bytecode to take the shared
function info as a parameter. It results in smaller code and avoids relying on a
previous bytecode to extract it.

Michi: The tests are updated as suggested but haven't managed to get the
isolate()->object_function()->shared() change to work as the isolate appears to
be missing a native context. Given we're not intending to write any more of
these tests, is this okay?

Thanks

https://codereview.chromium.org/1458603012/diff/100001/test/unittests/compile...
File test/unittests/compiler/bytecode-graph-builder-unittest.cc (right):

https://codereview.chromium.org/1458603012/diff/100001/test/unittests/compile...
test/unittests/compiler/bytecode-graph-builder-unittest.cc:842:
Handle<SharedFunctionInfo> shared_info = inner_graph_and_shared_info.second;
On 2015/11/23 11:18:38, Michael Starzinger wrote:
> If the only thing needed from the inner bytecode is a the "shared_info", then
I
> would vote for just using "isolate()->object_function()->shared()" instead.

This relies on having a native context. Not clear whether it's a bug that there
isn't one here.

https://codereview.chromium.org/1458603012/diff/100001/test/unittests/compile...
File test/unittests/compiler/node-test-utils.cc (right):

https://codereview.chromium.org/1458603012/diff/100001/test/unittests/compile...
test/unittests/compiler/node-test-utils.cc:1925: return
(PrintMatchAndExplain(&parameters, "value", parameters_matcher_,
On 2015/11/23 11:18:38, Michael Starzinger wrote:
> Instead of going through extra lengths of making the "CreateClosureParameters"
> comparable, can we instead just extract it from the node and compare the
values
> within? This would make the entire CreateClosureParametersMatcher obsolete.
This
> is similar to what we do in other matcher, IsJSLoadNamedMatcher for example.

Done.

[Benedikt Meurer, 2015-11-24 12:32:43]

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

[Benedikt Meurer, 2015-11-24 12:32:44]

Thanks for changing. LGTM from my side.

[mythria, 2015-11-24 13:07:28]

lgtm

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

https://codereview.chromium.org/1458603012/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:8: #include
"src/compiler/node-matchers.h"
This may not be needed now.

[Michael Starzinger, 2015-11-24 13:51:24]

LGTM with final nits.

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

https://codereview.chromium.org/1458603012/diff/120001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:8: #include
"src/compiler/node-matchers.h"
On 2015/11/24 13:07:28, mythria wrote:
> This may not be needed now.

+1

https://codereview.chromium.org/1458603012/diff/120001/test/unittests/compile...
File test/unittests/compiler/node-test-utils.h (right):

https://codereview.chromium.org/1458603012/diff/120001/test/unittests/compile...
test/unittests/compiler/node-test-utils.h:8: #include
"src/compiler/js-operator.h"
The include shouldn't be needed anymore, let's drop it again.

[oth, 2015-11-24 17:50:11]

The CQ bit was checked by oth@chromium.org

[oth, 2015-11-24 17:50:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mythria@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1458603012/#ps140001
(title: "Remove unneeded includes.")

[commit-bot: I haz the power, 2015-11-24 17:50:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458603012/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458603012/140001

[commit-bot: I haz the power, 2015-11-24 17:51:35]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-11-24 17:51:58]

Patchset 8 (id:??) landed as
https://crrev.com/4cceb11b0929abcbc82bf0854554a9b66003335d
Cr-Commit-Position: refs/heads/master@{#32224}

[oth, 2015-11-24 18:10:28]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1471913004/ by oth@chromium.org.

The reason for reverting is: Build break..