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

Issue 1391023007: [turbofan] Splinter into one range. (Closed)

Created:
5 years, 2 months ago by Mircea Trofin
Modified:
5 years, 2 months ago
CC:
v8-reviews_googlegroups.com, Jim Stichnoth
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[turbofan] Splinter into one range. Before this CL, we created one live range per successive set of deferred blocks. For scenarios with many such blocks, this creates an upfront pressure for the register allocator to deal with many ranges. Linear sorts ranges, which is a super-linear operation. The change places all deferred intervals into one range, meaning that, at most, there will be twice as many live ranges as the original set. In pathological cases (benchmarks/Compile/slow_nbody1.js), this change halves the compilation time. We see some improvements elsewhere, notably SQLite at ~4-5%. We may be able to avoid the subsequent merge. Its cost is the additional ranges it may need to create. The sole reason for the merge phase is to provide an unchanged view of the world to the subsequent phases. With the at-most-one splinter model, we may be able to teach the other phases about splintering - should we find perf hindrances due to merging. Committed: https://crrev.com/efdcd20267870276c5824f1ccf4e171ac378f7ae Cr-Commit-Position: refs/heads/master@{#31224}

Patch Set 1 : #

Patch Set 2 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+129 lines, -61 lines) Patch
M src/compiler/live-range-separator.cc View 1 chunk +7 lines, -5 lines 0 comments Download
M src/compiler/register-allocator.h View 1 4 chunks +15 lines, -4 lines 0 comments Download
M src/compiler/register-allocator.cc View 1 5 chunks +79 lines, -47 lines 1 comment Download
M test/unittests/compiler/live-range-unittest.cc View 1 3 chunks +28 lines, -5 lines 0 comments Download

Messages

Total messages: 12 (5 generated)
Mircea Trofin
5 years, 2 months ago (2015-10-12 23:28:39 UTC) #5
Benedikt Meurer
lgtm
5 years, 2 months ago (2015-10-13 03:53:42 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391023007/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391023007/80001
5 years, 2 months ago (2015-10-13 03:56:29 UTC) #8
commit-bot: I haz the power
Committed patchset #2 (id:80001)
5 years, 2 months ago (2015-10-13 03:58:04 UTC) #9
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/efdcd20267870276c5824f1ccf4e171ac378f7ae Cr-Commit-Position: refs/heads/master@{#31224}
5 years, 2 months ago (2015-10-13 03:58:24 UTC) #10
Benedikt Meurer
https://codereview.chromium.org/1391023007/diff/80001/src/compiler/register-allocator.cc File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/1391023007/diff/80001/src/compiler/register-allocator.cc#newcode981 src/compiler/register-allocator.cc:981: LiveRange* temp = first->SplitAt(second->Start(), zone); I ran into an ...
5 years, 2 months ago (2015-10-15 13:26:54 UTC) #11
Benedikt Meurer
5 years, 2 months ago (2015-10-15 13:27:46 UTC) #12
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:80001) has been created in
https://codereview.chromium.org/1403163003/ by bmeurer@chromium.org.

The reason for reverting is: Weird endless loop in TopLevelLiveRange::Merge()
due to always splitting first and not making progress. See comments,
unfortunately no useable repro..

Powered by Google App Engine
This is Rietveld 408576698