Provide optimized support for the %GetOrdinaryHasInstance intrinsic.

Provide optimized support for the %GetOrdinaryHasInstance intrinsic.

This new intrinsic is used by the desugared ES6 instanceof implementation for
the cases when the F[@@hasInstance] property is null or undefined.

R=mstarzinger@chromium.org

Committed: https://crrev.com/992ae64de0b3539ab59235c7700426aa008b607f
Cr-Commit-Position: refs/heads/master@{#34866}

[mvstanton, 2016-03-17 12:22:25]

mvstanton@chromium.org changed reviewers:
+ verwaest@chromium.org

[mvstanton, 2016-03-17 12:22:26]

Hi Michael,
Here is the CL we've been discussing, thanks for the info!

Toon: could you verify that the hydrogen changes are correct for such a new
intrinsic?

Thanks,
--Mike

[Michael Starzinger, 2016-03-17 12:43:53]

LGTM with comments and nits.

https://codereview.chromium.org/1809993002/diff/40001/src/compiler/linkage.cc
File src/compiler/linkage.cc (right):

https://codereview.chromium.org/1809993002/diff/40001/src/compiler/linkage.cc...
src/compiler/linkage.cc:171: case Runtime::kGetOrdinaryHasInstance:
As discussed offline: Since we are never emitting a runtime call to this
function within the compiler, this entry is not needed. We try to keep this list
as minimal as possible. Let's just remove it.

https://codereview.chromium.org/1809993002/diff/40001/test/unittests/compiler...
File test/unittests/compiler/js-intrinsic-lowering-unittest.cc (right):

https://codereview.chromium.org/1809993002/diff/40001/test/unittests/compiler...
test/unittests/compiler/js-intrinsic-lowering-unittest.cc:278: //
%_GetOrdinaryHasInstance
nit: Let's not squeeze this in between the test for Math functions. Can we just
move it to the bottom of the file?

https://codereview.chromium.org/1809993002/diff/40001/test/unittests/compiler...
test/unittests/compiler/js-intrinsic-lowering-unittest.cc:287:
ASSERT_TRUE(r.Changed());
nit: Let's also check that ...

EXPECT_THAT(r.replacement(), IsLoadContext(ContextAccess(0,
Context::ORDINARY_HAS_INSTANCE_INDEX, true), _));

[mvstanton, 2016-03-17 13:23:03]

Thanks Michael, most awesome...

https://codereview.chromium.org/1809993002/diff/40001/src/compiler/linkage.cc
File src/compiler/linkage.cc (right):

https://codereview.chromium.org/1809993002/diff/40001/src/compiler/linkage.cc...
src/compiler/linkage.cc:171: case Runtime::kGetOrdinaryHasInstance:
On 2016/03/17 12:43:53, Michael Starzinger wrote:
> As discussed offline: Since we are never emitting a runtime call to this
> function within the compiler, this entry is not needed. We try to keep this
list
> as minimal as possible. Let's just remove it.

Got it, and thx for the explanation.

https://codereview.chromium.org/1809993002/diff/40001/test/unittests/compiler...
File test/unittests/compiler/js-intrinsic-lowering-unittest.cc (right):

https://codereview.chromium.org/1809993002/diff/40001/test/unittests/compiler...
test/unittests/compiler/js-intrinsic-lowering-unittest.cc:278: //
%_GetOrdinaryHasInstance
On 2016/03/17 12:43:53, Michael Starzinger wrote:
> nit: Let's not squeeze this in between the test for Math functions. Can we
just
> move it to the bottom of the file?

Done.

https://codereview.chromium.org/1809993002/diff/40001/test/unittests/compiler...
test/unittests/compiler/js-intrinsic-lowering-unittest.cc:287:
ASSERT_TRUE(r.Changed());
On 2016/03/17 12:43:53, Michael Starzinger wrote:
> nit: Let's also check that ...
> 
> EXPECT_THAT(r.replacement(), IsLoadContext(ContextAccess(0,
> Context::ORDINARY_HAS_INSTANCE_INDEX, true), _));

Done.

[Toon Verwaest, 2016-03-17 15:12:54]

lgtm

https://codereview.chromium.org/1809993002/diff/60001/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1809993002/diff/60001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:12892:
handle(isolate()->native_context()->ordinary_has_instance()));
I think you can just do isolate()->ordinary_has_instance()

[mvstanton, 2016-03-17 15:23:14]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-03-17 15:23:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1809993002/#ps80001
(title: "REBASE and nit.")

[commit-bot: I haz the power, 2016-03-17 15:23:21]

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

[mvstanton, 2016-03-17 15:23:48]

Thanks Toon!
--Mike

https://codereview.chromium.org/1809993002/diff/60001/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1809993002/diff/60001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:12892:
handle(isolate()->native_context()->ordinary_has_instance()));
On 2016/03/17 15:12:54, Toon Verwaest wrote:
> I think you can just do isolate()->ordinary_has_instance()

Done.

[commit-bot: I haz the power, 2016-03-17 15:45:53]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-17 15:47:08]

Description was changed from

==========
Provide optimized support for the %GetOrdinaryHasInstance intrinsic.

This new intrinsic is used by the desugared ES6 instanceof implementation for
the cases when the F[@@hasInstance] property is null or undefined.

R=mstarzinger@chromium.org
==========

to

==========
Provide optimized support for the %GetOrdinaryHasInstance intrinsic.

This new intrinsic is used by the desugared ES6 instanceof implementation for
the cases when the F[@@hasInstance] property is null or undefined.

R=mstarzinger@chromium.org

Committed: https://crrev.com/992ae64de0b3539ab59235c7700426aa008b607f
Cr-Commit-Position: refs/heads/master@{#34866}
==========

[commit-bot: I haz the power, 2016-03-17 15:47:09]

Patchset 5 (id:??) landed as
https://crrev.com/992ae64de0b3539ab59235c7700426aa008b607f
Cr-Commit-Position: refs/heads/master@{#34866}