Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

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

Created:
1 year ago by Benedikt Meurer
Modified:
1 year 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

Messages

Total messages: 14 (6 generated)
Benedikt Meurer
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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 ...
1 year 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
1 year ago (2016-11-18 06:29:01 UTC) #11
commit-bot: I haz the power
Committed patchset #1 (id:1)
1 year ago (2016-11-18 06:31:04 UTC) #12
commit-bot: I haz the power
1 year 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}

Powered by Google App Engine
This is Rietveld 0eb02b776