[turbofan] Pseudo-inline instanceof

[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof' that is used if the "class", i.e. the constructor function, is a known constant.

Unittests check that replacement occurs as intended. Functional correctness is ensured by extensive unit tests covering instanceof already in the testsuite.

TESTS=unittests/JSTypedLoweringTest.{JSInstanceOfSpecializationWithSmiCheck,JSInstanceOfSpecializationWithoutSmiCheck,JSInstanceOfNoSpecialization}

Committed: https://crrev.com/45787501e5b9d0e225210ef9d2837b8ca6d8d73d
Cr-Commit-Position: refs/heads/master@{#31916}

[sigurds, 2015-11-06 16:07:48]

Description was changed from

==========
[turbofan] Pseudo-inline InstanceOf

This patch inlines InstanceOf if the 'class', i.e. the constructor
function is known.

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

to

==========
[turbofan] Pseudo-inline InstanceOf

This patch inlines InstanceOf if the 'class', i.e. the constructor
function is known.

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

[sigurds, 2015-11-06 16:07:49]

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

[sigurds, 2015-11-06 16:08:47]

Description was changed from

==========
[turbofan] Pseudo-inline InstanceOf

This patch inlines InstanceOf if the 'class', i.e. the constructor
function is known.

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

to

==========
[turbofan] Pseudo-inline 'instanceof'

This patch inlines 'instanceof' if the "class", i.e. the constructor function is
known.
==========

[sigurds, 2015-11-06 16:09:08]

Description was changed from

==========
[turbofan] Pseudo-inline 'instanceof'

This patch inlines 'instanceof' if the "class", i.e. the constructor function is
known.
==========

to

==========
[turbofan] Pseudo-inline 'instanceof'

This patch inlines 'instanceof' if the "class", i.e. the constructor function is
a known constant.
==========

[sigurds, 2015-11-09 17:29:05]

Description was changed from

==========
[turbofan] Pseudo-inline 'instanceof'

This patch inlines 'instanceof' if the "class", i.e. the constructor function is
a known constant.
==========

to

==========
[turbofan] Pseudo-inline 'instanceof'

This patch inlines 'instanceof' if the "class", i.e. the constructor function is
a known constant.

A cctest checks that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instancof in the testsuite.

TEST=cctest/js-typed-lowering/InstanceOfSpecialization
==========

[sigurds, 2015-11-09 17:30:03]

Description was changed from

==========
[turbofan] Pseudo-inline 'instanceof'

This patch inlines 'instanceof' if the "class", i.e. the constructor function is
a known constant.

A cctest checks that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instancof in the testsuite.

TEST=cctest/js-typed-lowering/InstanceOfSpecialization
==========

to

==========
[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof'
that is used if the "class", i.e. the constructor function, is a known constant.

A cctest checks that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instancof in the testsuite.

TEST=cctest/js-typed-lowering/InstanceOfSpecialization
==========

[sigurds, 2015-11-09 17:32:32]

Please take a look :)

[Benedikt Meurer, 2015-11-09 19:24:39]

First round of comments.

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.cc:1081:
this->dependencies()->AssumeInitialMapCantChange(initial_map);
This is only valid if deoptimization support is enabled. You need to pass in
flags to typed lowering, similar to what we do in JSNativeContextSpecialization.

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.cc:1088: if (!r.left_type()->Is(Type::Object()))
{
This should be done using if (r.left_type()->Maybe(Type::TaggedSigned()))

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.cc:1156: if (!r.left_type()->Is(Type::Object()))
{
How about if (if_is_smi != nullptr) instead?

[Michael Starzinger, 2015-11-09 19:59:59]

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
File src/compiler/js-typed-lowering.h (right):

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.h:8: #include "src/compilation-dependencies.h"
nit: The class can be forward-declared, no need for the include this in the
header.

https://codereview.chromium.org/1407413014/diff/100001/test/cctest/compiler/t...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/1407413014/diff/100001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:1274:
TEST(InstanceOfSpecialization) {
Since these tests only check that "something is reduced", I would vote for
turning them into real unit tests and moving them into
js-typed-lowering-unittest.cc. The FunctionTester is not needed, for the
constant function you can just use "isolate->object_function()" for example.

[sigurds, 2015-11-10 09:39:45]

Thanks for the comments, please take a look again

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.cc:1081:
this->dependencies()->AssumeInitialMapCantChange(initial_map);
On 2015/11/09 19:24:39, Benedikt Meurer wrote:
> This is only valid if deoptimization support is enabled. You need to pass in
> flags to typed lowering, similar to what we do in
JSNativeContextSpecialization

Done. I assume without deopt support, we can't do anything?

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.cc:1088: if (!r.left_type()->Is(Type::Object()))
{
On 2015/11/09 19:24:39, Benedikt Meurer wrote:
> This should be done using if (r.left_type()->Maybe(Type::TaggedSigned()))

Thanks, I did not know what the best check here was.

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.cc:1156: if (!r.left_type()->Is(Type::Object()))
{
On 2015/11/09 19:24:39, Benedikt Meurer wrote:
> How about if (if_is_smi != nullptr) instead?

Done.

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
File src/compiler/js-typed-lowering.h (right):

https://codereview.chromium.org/1407413014/diff/100001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.h:8: #include "src/compilation-dependencies.h"
On 2015/11/09 19:59:59, Michael Starzinger wrote:
> nit: The class can be forward-declared, no need for the include this in the
> header.

Done.

https://codereview.chromium.org/1407413014/diff/100001/test/cctest/compiler/t...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/1407413014/diff/100001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:1274:
TEST(InstanceOfSpecialization) {
On 2015/11/09 19:59:59, Michael Starzinger wrote:
> Since these tests only check that "something is reduced", I would vote for
> turning them into real unit tests and moving them into
> js-typed-lowering-unittest.cc. The FunctionTester is not needed, for the
> constant function you can just use "isolate->object_function()" for example.

Done.

[Michael Starzinger, 2015-11-10 09:48:48]

Looking good, just some final nits about the tests.

https://codereview.chromium.org/1407413014/diff/120001/test/cctest/compiler/t...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/1407413014/diff/120001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:17: #include
"test/cctest/compiler/function-tester.h"
nit: Include no longer needed.

https://codereview.chromium.org/1407413014/diff/120001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:137:
DCHECK_EQ(op->ValueInputCount(), 2);
nit: Change no longer needed, let's drop it.

https://codereview.chromium.org/1407413014/diff/120001/test/unittests/compile...
File test/unittests/compiler/js-typed-lowering-unittest.cc (right):

https://codereview.chromium.org/1407413014/diff/120001/test/unittests/compile...
test/unittests/compiler/js-typed-lowering-unittest.cc:1177: // InstanceOf
nit: s/InstanceOf/JSInstanceOf

https://codereview.chromium.org/1407413014/diff/120001/test/unittests/compile...
test/unittests/compiler/js-typed-lowering-unittest.cc:1182:
TEST_F(JSTypedLoweringTest, InstanceOfSpecialization) {
nit: Let's split this into three separate unit test cases. I like that fact that
our unit test are as simple and as minimal as possible.

[Michael Starzinger, 2015-11-10 09:49:49]

https://codereview.chromium.org/1407413014/diff/120001/test/unittests/compile...
File test/unittests/compiler/js-typed-lowering-unittest.cc (right):

https://codereview.chromium.org/1407413014/diff/120001/test/unittests/compile...
test/unittests/compiler/js-typed-lowering-unittest.cc:1182:
TEST_F(JSTypedLoweringTest, InstanceOfSpecialization) {
nit: s/InstanceOfSpecialization/JSInstanceOf/ here as well.

[sigurds, 2015-11-10 10:01:24]

PTALA :)

https://codereview.chromium.org/1407413014/diff/120001/test/cctest/compiler/t...
File test/cctest/compiler/test-js-typed-lowering.cc (right):

https://codereview.chromium.org/1407413014/diff/120001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:17: #include
"test/cctest/compiler/function-tester.h"
On 2015/11/10 09:48:48, Michael Starzinger wrote:
> nit: Include no longer needed.

Done.

https://codereview.chromium.org/1407413014/diff/120001/test/cctest/compiler/t...
test/cctest/compiler/test-js-typed-lowering.cc:137:
DCHECK_EQ(op->ValueInputCount(), 2);
On 2015/11/10 09:48:48, Michael Starzinger wrote:
> nit: Change no longer needed, let's drop it.

Done.

https://codereview.chromium.org/1407413014/diff/120001/test/unittests/compile...
File test/unittests/compiler/js-typed-lowering-unittest.cc (right):

https://codereview.chromium.org/1407413014/diff/120001/test/unittests/compile...
test/unittests/compiler/js-typed-lowering-unittest.cc:1177: // InstanceOf
On 2015/11/10 09:48:48, Michael Starzinger wrote:
> nit: s/InstanceOf/JSInstanceOf

Done.

https://codereview.chromium.org/1407413014/diff/120001/test/unittests/compile...
test/unittests/compiler/js-typed-lowering-unittest.cc:1182:
TEST_F(JSTypedLoweringTest, InstanceOfSpecialization) {
On 2015/11/10 09:48:48, Michael Starzinger wrote:
> nit: Let's split this into three separate unit test cases. I like that fact
that
> our unit test are as simple and as minimal as possible.

Done.

https://codereview.chromium.org/1407413014/diff/120001/test/unittests/compile...
test/unittests/compiler/js-typed-lowering-unittest.cc:1182:
TEST_F(JSTypedLoweringTest, InstanceOfSpecialization) {
On 2015/11/10 09:49:49, Michael Starzinger wrote:
> nit: s/InstanceOfSpecialization/JSInstanceOf/ here as well.

Done.

[Michael Starzinger, 2015-11-10 10:23:04]

LGTM from my end.

[Benedikt Meurer, 2015-11-10 11:29:06]

LGTM when comment is addressed.

https://codereview.chromium.org/1407413014/diff/160001/src/compiler/js-typed-...
File src/compiler/js-typed-lowering.h (right):

https://codereview.chromium.org/1407413014/diff/160001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.h:113: 
Please add

DEFINE_OPERATORS_FOR_FLAGS(JSTypedLowering::Flags)

here (see js-native-context-specialization.h).

[sigurds, 2015-11-10 12:03:12]

Running try jobs once, then using CQ to submit.

https://codereview.chromium.org/1407413014/diff/160001/src/compiler/js-typed-...
File src/compiler/js-typed-lowering.h (right):

https://codereview.chromium.org/1407413014/diff/160001/src/compiler/js-typed-...
src/compiler/js-typed-lowering.h:113: 
On 2015/11/10 11:29:06, Benedikt Meurer wrote:
> Please add
> 
> DEFINE_OPERATORS_FOR_FLAGS(JSTypedLowering::Flags)
> 
> here (see js-native-context-specialization.h).

Done.

[sigurds, 2015-11-10 12:06:26]

Description was changed from

==========
[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof'
that is used if the "class", i.e. the constructor function, is a known constant.

A cctest checks that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instancof in the testsuite.

TEST=cctest/js-typed-lowering/InstanceOfSpecialization
==========

to

==========
[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof'
that is used if the "class", i.e. the constructor function, is a known constant.

A cctest checks that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instancof in the testsuite.

TESTS=unittests/JSTypedLoweringTest.{JSInstanceOfSpecializationWithSmiCheck,JSInstanceOfSpecializationWithoutSmiCheck,JSInstanceOfNoSpecialization}
==========

[sigurds, 2015-11-10 12:06:47]

Description was changed from

==========
[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof'
that is used if the "class", i.e. the constructor function, is a known constant.

A cctest checks that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instancof in the testsuite.

TESTS=unittests/JSTypedLoweringTest.{JSInstanceOfSpecializationWithSmiCheck,JSInstanceOfSpecializationWithoutSmiCheck,JSInstanceOfNoSpecialization}
==========

to

==========
[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof'
that is used if the "class", i.e. the constructor function, is a known constant.

Unittests check that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instancof in the testsuite.

TESTS=unittests/JSTypedLoweringTest.{JSInstanceOfSpecializationWithSmiCheck,JSInstanceOfSpecializationWithoutSmiCheck,JSInstanceOfNoSpecialization}
==========

[sigurds, 2015-11-10 12:07:06]

Description was changed from

==========
[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof'
that is used if the "class", i.e. the constructor function, is a known constant.

Unittests check that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instancof in the testsuite.

TESTS=unittests/JSTypedLoweringTest.{JSInstanceOfSpecializationWithSmiCheck,JSInstanceOfSpecializationWithoutSmiCheck,JSInstanceOfNoSpecialization}
==========

to

==========
[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof'
that is used if the "class", i.e. the constructor function, is a known constant.

Unittests check that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instanceof in the testsuite.

TESTS=unittests/JSTypedLoweringTest.{JSInstanceOfSpecializationWithSmiCheck,JSInstanceOfSpecializationWithoutSmiCheck,JSInstanceOfNoSpecialization}
==========

[sigurds, 2015-11-10 12:07:17]

Description was changed from

==========
[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof'
that is used if the "class", i.e. the constructor function, is a known constant.

Unittests check that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instanceof in the testsuite.

TESTS=unittests/JSTypedLoweringTest.{JSInstanceOfSpecializationWithSmiCheck,JSInstanceOfSpecializationWithoutSmiCheck,JSInstanceOfNoSpecialization}
==========

to

==========
[turbofan] Pseudo-inline 'instanceof'

This patch extends the typed lowering with a specialized version of 'instanceof'
that is used if the "class", i.e. the constructor function, is a known constant.

Unittests check that replacement occurs as intended. Functional correctness is
ensured by extensive unit tests covering instanceof already in the testsuite.

TESTS=unittests/JSTypedLoweringTest.{JSInstanceOfSpecializationWithSmiCheck,JSInstanceOfSpecializationWithoutSmiCheck,JSInstanceOfNoSpecialization}
==========

[sigurds, 2015-11-10 12:07:42]

The CQ bit was checked by sigurds@chromium.org

[sigurds, 2015-11-10 12:07:43]

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

[commit-bot: I haz the power, 2015-11-10 12:07:49]

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

[commit-bot: I haz the power, 2015-11-10 12:19:41]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-10 12:20:23]

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