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

Issue 1472803004: [turbofan] Simplified splintering code. (Closed)

Created:
5 years, 1 month ago by Mircea Trofin
Modified:
5 years ago
Reviewers:
Benedikt Meurer, Jarin
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

A simpler way to determine if a range spills only in deferred blocks, by validating that the hot path does not spill - somewhat simpler code. Cleared the scenario where a range is defined in a deferred block. The code before was slightly more complicated by not leveraging the property that these sort of ranges would be completely contained within deferred blocks. Moved "spills in deferred blocks" marking to a more appropriate location. One thing this CL achieves is correct support for scenarios where a range is spilled both on the deferred and then hot path, and the ranges concatenate. I owe better unit testing, which I will add in a subsequent CL. BUG= Committed: https://crrev.com/be7e43614c1d7ce63a91b69d7a4bff01d5a74d10 Cr-Commit-Position: refs/heads/master@{#32302}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 4

Patch Set 5 : #

Total comments: 1

Patch Set 6 : typo #

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+93 lines, -82 lines) Patch
M src/compiler/live-range-separator.h View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M src/compiler/live-range-separator.cc View 1 2 3 4 3 chunks +26 lines, -23 lines 0 comments Download
M src/compiler/pipeline.cc View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M src/compiler/register-allocator.h View 1 2 3 4 5 3 chunks +23 lines, -1 line 0 comments Download
M src/compiler/register-allocator.cc View 1 2 3 4 5 4 chunks +37 lines, -58 lines 0 comments Download

Messages

Total messages: 13 (6 generated)
Mircea Trofin
5 years, 1 month ago (2015-11-24 06:04:10 UTC) #3
Jarin
https://codereview.chromium.org/1472803004/diff/60001/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/1472803004/diff/60001/src/compiler/pipeline.cc#newcode847 src/compiler/pipeline.cc:847: struct MarkSpilledOnlyInDeferredPhase { I think there is no need ...
5 years ago (2015-11-25 10:33:08 UTC) #4
Mircea Trofin
Addressed feedback. https://codereview.chromium.org/1472803004/diff/60001/src/compiler/pipeline.cc File src/compiler/pipeline.cc (right): https://codereview.chromium.org/1472803004/diff/60001/src/compiler/pipeline.cc#newcode847 src/compiler/pipeline.cc:847: struct MarkSpilledOnlyInDeferredPhase { On 2015/11/25 10:33:08, Jarin ...
5 years ago (2015-11-25 16:21:36 UTC) #5
Jarin
lgtm. https://codereview.chromium.org/1472803004/diff/80001/src/compiler/register-allocator.h File src/compiler/register-allocator.h (right): https://codereview.chromium.org/1472803004/diff/80001/src/compiler/register-allocator.h#newcode795 src/compiler/register-allocator.h:795: bool RangesDefinedInDeferredStaysInDeferred(); Stays -> Stay
5 years ago (2015-11-25 19:23:39 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472803004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472803004/100001
5 years ago (2015-11-25 19:31:48 UTC) #9
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years ago (2015-11-25 20:07:59 UTC) #11
commit-bot: I haz the power
5 years ago (2015-11-25 20:08:18 UTC) #13
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/be7e43614c1d7ce63a91b69d7a4bff01d5a74d10
Cr-Commit-Position: refs/heads/master@{#32302}

Powered by Google App Engine
This is Rietveld 408576698