Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2749023005:
[turbofan] Try to allocate preferred register after splitting range. (Closed)
|
Created:
3 years, 9 months ago by georgia.kouveli Modified:
3 years, 9 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Try to allocate preferred register after splitting range.
This shows an improvement in the code size of the bytecode handlers.
When a range is split (because for example the preferred register gets
clobbered by a call and is not available for the whole range), trying
to allocate the preferred register for the first range that results
from the split avoids some extra register moves.
BUG=
Review-Url: https://codereview.chromium.org/2749023005
Cr-Commit-Position: refs/heads/master@{#43905}
Committed: https://chromium.googlesource.com/v8/v8/+/9e074945096dbf309e0542dfb6c30f4cf6624cc4
Patch Set 1 #
Total comments: 2
Patch Set 2 : [turbofan] Try to allocate preferred register after splitting range. #Messages
Total messages: 18 (9 generated)
Description was changed from ========== [turbofan] Try to allocate preferred register after splitting range. This shows an improvement in the code size of the bytecode handlers. When a range is split (because for example the preferred register gets clobbered by a call and is not available for the whole range), trying to allocate the preferred register for the first range that results from the split avoids some extra register moves. BUG= ========== to ========== [turbofan] Try to allocate preferred register after splitting range. This shows an improvement in the code size of the bytecode handlers. When a range is split (because for example the preferred register gets clobbered by a call and is not available for the whole range), trying to allocate the preferred register for the first range that results from the split avoids some extra register moves. BUG= ==========
georgia.kouveli@arm.com changed reviewers: + bmeurer@chromium.org
bmeurer@chromium.org changed reviewers: + jarin@chromium.org, mtrofin@chromium.org
bmeurer@chromium.org changed required reviewers: + jarin@chromium.org
jarin@ and mtrofin@ should review this.
jarin@chromium.org changed required reviewers: + mtrofin@chromium.org
lgtm, let's wait for Mircea's opinion. https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... src/compiler/register-allocator.cc:3156: if (TryAllocatePreferredReg(current, free_until_pos)) return true; Could this go into the then-branch above? (before line 3153)
https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... src/compiler/register-allocator.cc:3156: if (TryAllocatePreferredReg(current, free_until_pos)) return true; On 2017/03/16 06:24:28, Jarin wrote: > Could this go into the then-branch above? (before line 3153) > Done.
On 2017/03/16 14:07:32, georgia.kouveli wrote: > https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... > File src/compiler/register-allocator.cc (right): > > https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... > src/compiler/register-allocator.cc:3156: if (TryAllocatePreferredReg(current, > free_until_pos)) return true; > On 2017/03/16 06:24:28, Jarin wrote: > > Could this go into the then-branch above? (before line 3153) > > > > Done. lgtm. This may address https://bugs.chromium.org/p/v8/issues/detail?id=6034. Would you mind verifying, and, if so, closing that bug, too?
The CQ bit was checked by georgia.kouveli@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org Link to the patchset: https://codereview.chromium.org/2749023005/#ps20001 (title: "[turbofan] Try to allocate preferred register after splitting range.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/17 13:42:30, Mircea Trofin wrote: > On 2017/03/16 14:07:32, georgia.kouveli wrote: > > > https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... > > File src/compiler/register-allocator.cc (right): > > > > > https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... > > src/compiler/register-allocator.cc:3156: if (TryAllocatePreferredReg(current, > > free_until_pos)) return true; > > On 2017/03/16 06:24:28, Jarin wrote: > > > Could this go into the then-branch above? (before line 3153) > > > > > > > Done. > > lgtm. > > This may address https://bugs.chromium.org/p/v8/issues/detail?id=6034. Would you > mind verifying, and, if so, closing that bug, too? Thanks! I've checked and the issue described in 6034 is still present with my patch.
On 2017/03/17 14:26:35, georgia.kouveli wrote: > On 2017/03/17 13:42:30, Mircea Trofin wrote: > > On 2017/03/16 14:07:32, georgia.kouveli wrote: > > > > > > https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... > > > File src/compiler/register-allocator.cc (right): > > > > > > > > > https://codereview.chromium.org/2749023005/diff/1/src/compiler/register-alloc... > > > src/compiler/register-allocator.cc:3156: if > (TryAllocatePreferredReg(current, > > > free_until_pos)) return true; > > > On 2017/03/16 06:24:28, Jarin wrote: > > > > Could this go into the then-branch above? (before line 3153) > > > > > > > > > > Done. > > > > lgtm. > > > > This may address https://bugs.chromium.org/p/v8/issues/detail?id=6034. Would > you > > mind verifying, and, if so, closing that bug, too? > > Thanks! I've checked and the issue described in 6034 is still present with my > patch. Thanks!
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489760067306050,
"parent_rev": "94efcfe01be03b3dfc0a8b335aca9d3246ddc016", "commit_rev":
"9e074945096dbf309e0542dfb6c30f4cf6624cc4"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Try to allocate preferred register after splitting range. This shows an improvement in the code size of the bytecode handlers. When a range is split (because for example the preferred register gets clobbered by a call and is not available for the whole range), trying to allocate the preferred register for the first range that results from the split avoids some extra register moves. BUG= ========== to ========== [turbofan] Try to allocate preferred register after splitting range. This shows an improvement in the code size of the bytecode handlers. When a range is split (because for example the preferred register gets clobbered by a call and is not available for the whole range), trying to allocate the preferred register for the first range that results from the split avoids some extra register moves. BUG= Review-Url: https://codereview.chromium.org/2749023005 Cr-Commit-Position: refs/heads/master@{#43905} Committed: https://chromium.googlesource.com/v8/v8/+/9e074945096dbf309e0542dfb6c30f4cf66... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/9e074945096dbf309e0542dfb6c30f4cf66... |
