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

Issue 988423004: [turbofan] Context specialize during inlining. (Closed)

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

Description

[turbofan] Context specialize during inlining. Context specialization enables inlining (at least currently it is the only enabler for inlining), but inlining enables more possibilities for context specialization. So we really need to run them together. This is especially important with the "module based builtins" that we're working towards. BUG=v8:3952 LOG=n Committed: https://crrev.com/a556a104c0f2c3d9dc00c37ff3d6d93db5287354 Cr-Commit-Position: refs/heads/master@{#27085}

Patch Set 1 #

Patch Set 2 : Add test case for nested builtin inlining. #

Patch Set 3 : Ben's idea. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -13 lines) Patch
M src/compiler/js-inlining.cc View 1 2 6 chunks +16 lines, -13 lines 1 comment Download
M test/cctest/compiler/test-run-inlining.cc View 1 1 chunk +18 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (3 generated)
Benedikt Meurer
Yang: This is the change I was talking about to make your context based builtins ...
5 years, 9 months ago (2015-03-09 18:40:32 UTC) #2
Benedikt Meurer
Ben: This uses the approach you suggested, and is actually easier long-term. PTAL
5 years, 9 months ago (2015-03-10 08:10:15 UTC) #4
titzer
https://codereview.chromium.org/988423004/diff/40001/src/compiler/js-inlining.cc File src/compiler/js-inlining.cc (right): https://codereview.chromium.org/988423004/diff/40001/src/compiler/js-inlining.cc#newcode350 src/compiler/js-inlining.cc:350: graph_builder.CreateGraph(true, false); Can we context specialization a flag to ...
5 years, 9 months ago (2015-03-10 08:16:21 UTC) #5
titzer
Sigh LGTM
5 years, 9 months ago (2015-03-10 08:26:08 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988423004/40001
5 years, 9 months ago (2015-03-10 08:29:10 UTC) #8
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 9 months ago (2015-03-10 08:37:21 UTC) #9
commit-bot: I haz the power
5 years, 9 months ago (2015-03-10 08:37:32 UTC) #10
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/a556a104c0f2c3d9dc00c37ff3d6d93db5287354
Cr-Commit-Position: refs/heads/master@{#27085}

Powered by Google App Engine
This is Rietveld 408576698