Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(181)

Issue 2196653002: [turbofan] Introduce a dedicated CheckMaps simplified operator. (Closed)

Created:
4 years, 4 months ago by Benedikt Meurer
Modified:
4 years, 4 months ago
Reviewers:
epertoso
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@TurboFan_JSNativeContextSpecialization_NonElementKeyedAccess
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[turbofan] Introduce a dedicated CheckMaps simplified operator. So far we always create explicit control flow for map checks during JSNativeContextSpecialization, or in the monomorphic case we used a CheckIf combined with a map comparison. In either case we cannot currently effectively utilize the map check information during load elimination to optimize (polymorphic) map checks and elements kind transitions. With the introduction of CheckMaps, we can now start optimizing map checks in a more effective fashion. This CL doesn't change anything in that direction yet, but merely changes the fundamental mechanism. This also removes the stable map optimization from the Typer, where it was always a bit odd, and puts it into the typed lowering and the native context specialization instead. R=epertoso@chromium.org BUG=v8:4930, v8:5141 Committed: https://crrev.com/8201579e031aee4118e7a98ac0991418d01fd06a Cr-Commit-Position: refs/heads/master@{#38166}

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+452 lines, -219 lines) Patch
M src/compiler/effect-control-linearizer.h View 1 chunk +2 lines, -0 lines 0 comments Download
M src/compiler/effect-control-linearizer.cc View 2 chunks +40 lines, -0 lines 0 comments Download
M src/compiler/js-native-context-specialization.h View 1 chunk +7 lines, -0 lines 0 comments Download
M src/compiler/js-native-context-specialization.cc View 7 chunks +234 lines, -141 lines 2 comments Download
M src/compiler/js-typed-lowering.h View 1 chunk +2 lines, -0 lines 0 comments Download
M src/compiler/js-typed-lowering.cc View 4 chunks +83 lines, -0 lines 0 comments Download
M src/compiler/load-elimination.h View 1 chunk +2 lines, -0 lines 0 comments Download
M src/compiler/load-elimination.cc View 2 chunks +50 lines, -0 lines 0 comments Download
M src/compiler/opcodes.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/pipeline.cc View 1 chunk +1 line, -4 lines 0 comments Download
M src/compiler/simplified-lowering.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/simplified-operator.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/simplified-operator.cc View 1 chunk +12 lines, -0 lines 0 comments Download
M src/compiler/typer.h View 5 chunks +1 line, -17 lines 0 comments Download
M src/compiler/typer.cc View 5 chunks +7 lines, -57 lines 0 comments Download
M src/compiler/verifier.cc View 1 chunk +8 lines, -0 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 12 (6 generated)
Benedikt Meurer
4 years, 4 months ago (2016-07-29 07:34:53 UTC) #1
epertoso
lgtm https://codereview.chromium.org/2196653002/diff/1/src/compiler/js-native-context-specialization.cc File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2196653002/diff/1/src/compiler/js-native-context-specialization.cc#newcode543 src/compiler/js-native-context-specialization.cc:543: // Create single chokepoint for the control. nit: ...
4 years, 4 months ago (2016-07-29 09:34:08 UTC) #6
Benedikt Meurer
https://codereview.chromium.org/2196653002/diff/1/src/compiler/js-native-context-specialization.cc File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2196653002/diff/1/src/compiler/js-native-context-specialization.cc#newcode543 src/compiler/js-native-context-specialization.cc:543: // Create single chokepoint for the control. Yes.
4 years, 4 months ago (2016-07-29 09:37:49 UTC) #7
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/2196653002/1
4 years, 4 months ago (2016-07-29 09:38:19 UTC) #9
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 4 months ago (2016-07-29 09:39:39 UTC) #10
commit-bot: I haz the power
4 years, 4 months ago (2016-07-29 09:41:29 UTC) #12
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/8201579e031aee4118e7a98ac0991418d01fd06a
Cr-Commit-Position: refs/heads/master@{#38166}

Powered by Google App Engine
This is Rietveld 408576698