Chromium Code Reviews
Help | Chromium Project | Sign in
(16)

Issue 2511223003: [turbofan] Properly optimize instanceof (even in the presence of @@hasInstance). (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 1 week ago by Benedikt Meurer
Modified:
5 months, 1 week ago
Reviewers:
Yang
CC:
v8-reviews_googlegroups.com, Franzi
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[turbofan] Properly optimize instanceof (even in the presence of @@hasInstance). This is the TurboFan counterpart of http://crrev.com/2504263004, but it is a bit more involved, since in TurboFan we always inline the appropriate call to the @@hasInstance handler, and by that we can optimize a lot more patterns of instanceof than Crankshaft, and even yield fast instanceof for custom @@hasInstance handlers (which we can now properly inline as well). Also we now properly optimize Function.prototype[@@hasInstance], even if the right hand side of an instanceof doesn't have the Function.prototype as its direct prototype. For the baseline case, we still rely on the global protector cell, but we can address that in a follow-up as well, and make it more robust in general. TEST=mjsunit/compiler/instanceof BUG=v8:5640 R=yangguo@chromium.org Committed: https://crrev.com/241c024c10a8c1b5d7309299ef61f887363b00a0 Cr-Commit-Position: refs/heads/master@{#41092}

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+329 lines, -74 lines) Patch
M src/bootstrapper.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/builtins/builtins.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/builtins/builtins-object.cc View 1 chunk +14 lines, -0 lines 0 comments Download
M src/code-factory.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/code-factory.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M src/compiler/js-builtin-reducer.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/js-builtin-reducer.cc View 2 chunks +31 lines, -0 lines 0 comments Download
M src/compiler/js-generic-lowering.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M src/compiler/js-native-context-specialization.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/js-native-context-specialization.cc View 2 chunks +88 lines, -0 lines 4 comments Download
M src/compiler/js-operator.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/js-operator.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/js-typed-lowering.h View 1 chunk +1 line, -1 line 0 comments Download
M src/compiler/js-typed-lowering.cc View 4 chunks +32 lines, -35 lines 0 comments Download
M src/compiler/opcodes.h View 1 chunk +2 lines, -1 line 0 comments Download
M src/compiler/operator-properties.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/typer.cc View 2 chunks +10 lines, -0 lines 0 comments Download
M src/compiler/verifier.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/objects.h View 1 chunk +1 line, -0 lines 0 comments Download
A test/mjsunit/compiler/instanceof.js View 1 chunk +133 lines, -0 lines 0 comments Download
M test/unittests/compiler/js-typed-lowering-unittest.cc View 1 chunk +0 lines, -37 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 14 (6 generated)
Benedikt Meurer
5 months, 1 week ago (2016-11-18 05:52:34 UTC) #1
Benedikt Meurer
Hey Yang, Here's the appropriate TurboFan CL for the @@hasInstance fix. It is more general ...
5 months, 1 week ago (2016-11-18 05:55:08 UTC) #4
Yang
On 2016/11/18 05:55:08, Benedikt Meurer wrote: > Hey Yang, > > Here's the appropriate TurboFan ...
5 months, 1 week ago (2016-11-18 06:26:23 UTC) #7
Yang
https://codereview.chromium.org/2511223003/diff/1/src/compiler/js-native-context-specialization.cc File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2511223003/diff/1/src/compiler/js-native-context-specialization.cc#newcode135 src/compiler/js-native-context-specialization.cc:135: NodeProperties::ChangeOp(node, javascript()->OrdinaryHasInstance()); Would it make sense to constant fold ...
5 months, 1 week ago (2016-11-18 06:26:47 UTC) #8
Benedikt Meurer
https://codereview.chromium.org/2511223003/diff/1/src/compiler/js-native-context-specialization.cc File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2511223003/diff/1/src/compiler/js-native-context-specialization.cc#newcode135 src/compiler/js-native-context-specialization.cc:135: NodeProperties::ChangeOp(node, javascript()->OrdinaryHasInstance()); Doable, but as discussed offline, there's no ...
5 months, 1 week ago (2016-11-18 06:28:51 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2511223003/1
5 months, 1 week ago (2016-11-18 06:29:01 UTC) #11
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 months, 1 week ago (2016-11-18 06:31:04 UTC) #12
commit-bot: I haz the power
5 months, 1 week ago (2016-11-18 06:31:54 UTC) #14
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/241c024c10a8c1b5d7309299ef61f887363b00a0
Cr-Commit-Position: refs/heads/master@{#41092}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46