Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632373004:
[turbofan] Regalloc was assuming "blocked" register can't be "used" (Closed)
|
Created:
3 years, 11 months ago by Mircea Trofin Modified:
3 years, 11 months ago CC:
v8-reviews_googlegroups.com, ahaas, Michael Hablich Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Regalloc was assuming "blocked" register can't be "used"
When attempting to allocate a blocked register, in the absence of
aliasing, it was possible to assume that a register that was
blocked - by either belonging to an active fixed register, or to
an active unspillable range - could not have possibly be allocated
to another active range (because there'd be an interference otherwise).
With aliasing, that changes. The range we're trying to allocate
may be a double, while the 2 or more active ranges in the paragraph
above may be singles aliasing to the same double slot.
Opportunistically refactored for readability an optimization, and
added some comments.
BUG=681529
Review-Url: https://codereview.chromium.org/2632373004
Cr-Commit-Position: refs/heads/master@{#42474}
Committed: https://chromium.googlesource.com/v8/v8/+/970d90767dce7fa13e0f0aa5b4eaea83af710e65
Patch Set 1 #
Total comments: 4
Patch Set 2 : DCHECKS #Messages
Total messages: 30 (21 generated)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [turbofan] Regalloc was assuming "blocked" register can't be "used" When attempting to allocate a blocked register, in the absence of aliasing, it was possible to assume that a register that was blocked - by either belonging to an active fixed register, or to an active unspillable range - could not have possibly be allocated to another active range (because there'd be an interference otherwise). With aliasing, that changes. The range we're trying to allocate may be a double, while the 2 or more active ranges in the paragraph above may be singles aliasing to the same double slot. Opportunistically refactored for readability an optimization, and added some comments. BUG= ========== to ========== [turbofan] Regalloc was assuming "blocked" register can't be "used" When attempting to allocate a blocked register, in the absence of aliasing, it was possible to assume that a register that was blocked - by either belonging to an active fixed register, or to an active unspillable range - could not have possibly be allocated to another active range (because there'd be an interference otherwise). With aliasing, that changes. The range we're trying to allocate may be a double, while the 2 or more active ranges in the paragraph above may be singles aliasing to the same double slot. Opportunistically refactored for readability an optimization, and added some comments. BUG=681529 ==========
mtrofin@chromium.org changed reviewers: + bbudge@chromium.org, bmeurer@chromium.org, bradnelson@chromium.org, jarin@chromium.org
PTAL Verified fix locally. Opened 5856 to track testing for aliasing. Splitting the fix this way (tests separate) because of the blocking nature of the bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2632373004/diff/1/src/compiler/register-alloc... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2632373004/diff/1/src/compiler/register-alloc... src/compiler/register-allocator.cc:3198: range->NextLifetimePositionRegisterIsBeneficial(current->Start())); Is this necessary here, since there is either simple aliasing, or there is no FP aliasing?
https://codereview.chromium.org/2632373004/diff/1/src/compiler/register-alloc... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2632373004/diff/1/src/compiler/register-alloc... src/compiler/register-allocator.cc:3198: range->NextLifetimePositionRegisterIsBeneficial(current->Start())); On 2017/01/18 00:45:13, bbudge wrote: > Is this necessary here, since there is either simple aliasing, or there is no FP > aliasing? It's not, but I'd prefer it be there for consistency with the rest of the code. Alternatively, I could DCHECK there and also in the line above.
LGTM https://codereview.chromium.org/2632373004/diff/1/src/compiler/register-alloc... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2632373004/diff/1/src/compiler/register-alloc... src/compiler/register-allocator.cc:3198: range->NextLifetimePositionRegisterIsBeneficial(current->Start())); On 2017/01/18 01:10:12, Mircea Trofin wrote: > On 2017/01/18 00:45:13, bbudge wrote: > > Is this necessary here, since there is either simple aliasing, or there is no > FP > > aliasing? > > It's not, but I'd prefer it be there for consistency with the rest of the code. > > Alternatively, I could DCHECK there and also in the line above DCHECK seems clearer to me than calling Min which always returns NextLifetimePosition...
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2632373004/diff/1/src/compiler/register-alloc... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2632373004/diff/1/src/compiler/register-alloc... src/compiler/register-allocator.cc:3198: range->NextLifetimePositionRegisterIsBeneficial(current->Start())); On 2017/01/18 01:14:51, bbudge wrote: > On 2017/01/18 01:10:12, Mircea Trofin wrote: > > On 2017/01/18 00:45:13, bbudge wrote: > > > Is this necessary here, since there is either simple aliasing, or there is > no > > FP > > > aliasing? > > > > It's not, but I'd prefer it be there for consistency with the rest of the > code. > > > > Alternatively, I could DCHECK there and also in the line above > > DCHECK seems clearer to me than calling Min which always returns > NextLifetimePosition... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19386) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
mtrofin@chromium.org changed reviewers: + hablich@chromium.org - bmeurer@chromium.org
ping (I need a "compiler" OWNERS reviewer)
titzer@chromium.org changed reviewers: + titzer@chromium.org
lgtm
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/2632373004/#ps40001 (title: "DCHECKS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484765639489140,
"parent_rev": "aa3cd2cd0745266fe98a5fc1304a889de431bcea", "commit_rev":
"970d90767dce7fa13e0f0aa5b4eaea83af710e65"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Regalloc was assuming "blocked" register can't be "used" When attempting to allocate a blocked register, in the absence of aliasing, it was possible to assume that a register that was blocked - by either belonging to an active fixed register, or to an active unspillable range - could not have possibly be allocated to another active range (because there'd be an interference otherwise). With aliasing, that changes. The range we're trying to allocate may be a double, while the 2 or more active ranges in the paragraph above may be singles aliasing to the same double slot. Opportunistically refactored for readability an optimization, and added some comments. BUG=681529 ========== to ========== [turbofan] Regalloc was assuming "blocked" register can't be "used" When attempting to allocate a blocked register, in the absence of aliasing, it was possible to assume that a register that was blocked - by either belonging to an active fixed register, or to an active unspillable range - could not have possibly be allocated to another active range (because there'd be an interference otherwise). With aliasing, that changes. The range we're trying to allocate may be a double, while the 2 or more active ranges in the paragraph above may be singles aliasing to the same double slot. Opportunistically refactored for readability an optimization, and added some comments. BUG=681529 Review-Url: https://codereview.chromium.org/2632373004 Cr-Commit-Position: refs/heads/master@{#42474} Committed: https://chromium.googlesource.com/v8/v8/+/970d90767dce7fa13e0f0aa5b4eaea83af7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/v8/v8/+/970d90767dce7fa13e0f0aa5b4eaea83af7... |
