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

Issue 1244583003: [turbofan]: Add a context relaxer reducer (Closed)

Created:
5 years, 5 months ago by danno
Modified:
5 years, 5 months ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[turbofan]: Add a context relaxation Reducer In many cases, the context that TurboFan's ASTGraphBuilder or subsequent reduction operations attaches to nodes does not need to be that exact context, but rather only needs to be one with the same native context, because it is used internally only to fetch the native context, e.g. for creating and throwing exceptions. This reducer recognizes common cases where the context that is specified for a node can be relaxed to a canonical, less specific one. This relaxed context can either be the enclosing function's context or a specific Module or Script context that is explicitly created within the function. This optimization is especially important for TurboFan-generated code stubs which use context specialization and inlining to generate optimal code. Without context relaxation, many extraneous moves are generated to pass exactly the right context to internal functions like ToNumber and AllocateHeapNumber, which only need the native context. By turning context relaxation on, these moves disappear because all these common internal context uses are unified to the context passed into the stub function, which is typically already in the correct context register and remains there for short stubs. It also eliminates the explicit use of a specialized context constant in the code stub in these cases, which could cause memory leaks. Committed: https://crrev.com/cca5e74a5814543e7446167ba47bfa498ae10c16 Cr-Commit-Position: refs/heads/master@{#29763}

Patch Set 1 #

Patch Set 2 : Latest #

Total comments: 11

Patch Set 3 : Review feedback #

Patch Set 4 : Latest #

Patch Set 5 : Really this time #

Unified diffs Side-by-side diffs Delta from patch set Stats (+441 lines, -11 lines) Patch
M BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
M src/code-stubs.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/ast-graph-builder.cc View 1 1 chunk +3 lines, -3 lines 0 comments Download
M src/compiler/common-operator.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M src/compiler/common-operator.cc View 1 1 chunk +4 lines, -2 lines 0 comments Download
M src/compiler/frame-states.h View 1 1 chunk +14 lines, -2 lines 0 comments Download
A src/compiler/js-context-relaxation.h View 1 2 1 chunk +32 lines, -0 lines 0 comments Download
A src/compiler/js-context-relaxation.cc View 1 2 3 4 1 chunk +67 lines, -0 lines 0 comments Download
M src/compiler/js-inlining.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M src/compiler/pipeline.cc View 1 2 3 chunks +3 lines, -0 lines 0 comments Download
M test/unittests/compiler/instruction-selector-unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
A test/unittests/compiler/js-context-relaxation-unittest.cc View 1 2 1 chunk +306 lines, -0 lines 0 comments Download
M test/unittests/compiler/liveness-analyzer-unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M test/unittests/unittests.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M tools/gyp/v8.gyp View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (9 generated)
danno
PTAL. This implements the new strategy as we discussed.
5 years, 5 months ago (2015-07-17 14:25:51 UTC) #2
Michael Starzinger
LGTM, I like it. https://codereview.chromium.org/1244583003/diff/20001/src/compiler/js-context-relaxation.cc File src/compiler/js-context-relaxation.cc (right): https://codereview.chromium.org/1244583003/diff/20001/src/compiler/js-context-relaxation.cc#newcode30 src/compiler/js-context-relaxation.cc:30: return NoChange(); Could we just ...
5 years, 5 months ago (2015-07-20 08:09:49 UTC) #3
danno
Feedback addressed. Landing. https://codereview.chromium.org/1244583003/diff/20001/src/compiler/js-context-relaxation.cc File src/compiler/js-context-relaxation.cc (right): https://codereview.chromium.org/1244583003/diff/20001/src/compiler/js-context-relaxation.cc#newcode30 src/compiler/js-context-relaxation.cc:30: return NoChange(); On 2015/07/20 at 08:09:49, ...
5 years, 5 months ago (2015-07-20 15:27:33 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1244583003/40001
5 years, 5 months ago (2015-07-20 15:27:41 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1244583003/60001
5 years, 5 months ago (2015-07-20 16:44:57 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1244583003/80001
5 years, 5 months ago (2015-07-20 16:51:46 UTC) #15
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 5 months ago (2015-07-20 17:16:05 UTC) #16
commit-bot: I haz the power
5 years, 5 months ago (2015-07-20 17:16:26 UTC) #17
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/cca5e74a5814543e7446167ba47bfa498ae10c16
Cr-Commit-Position: refs/heads/master@{#29763}

Powered by Google App Engine
This is Rietveld 408576698