Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2347563004:
[turbofan] Avoid large deopt blocks (Closed)
|
Created:
4 years, 3 months ago by Mircea Trofin Modified:
4 years, 2 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Avoid large deopt blocks
Treat allocation of splintered ranges differently, by optimizing for move
counts (i.e. try to have less move counts), rather than optimizing for
quality of moves (which is what normal allocation does).
We can see reductions in code size in the benchmarks that measure it
(e.g. Unity)
BUG=
Committed: https://crrev.com/33629651584b669c0bd24a9be7bad868f01ea809
Cr-Commit-Position: refs/heads/master@{#40178}
Patch Set 1 #Patch Set 2 : Exclude splinters from pre-spilling #Patch Set 3 : Exclude splinters from pre-spilling #Patch Set 4 : A few more adjustments #Patch Set 5 : A few more adjustments #
Total comments: 15
Patch Set 6 : feedback #
Total comments: 4
Patch Set 7 : refactoring #
Total comments: 2
Patch Set 8 : format #Patch Set 9 : [turbofan] Avoid large deopt blocks #
Messages
Total messages: 45 (31 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Different treatment for splinters BUG= ========== to ========== [turbofan] Avoid large deopt blocks Treat allocation of splintered ranges differently, by optimizing for move counts (i.e. try to have less move counts), rather than optimizing for quality of moves (which is what normal allocation does). We can see reductions in code size in the benchmarks that measure it (e.g. Unity) BUG= ==========
mtrofin@chromium.org changed reviewers: + bmeurer@chromium.org, jarin@chromium.org
gentle reminder.
https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:590: // to/from nit: merge the comment with the next line. https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:735: other->FirstHintPosition() == nullptr) I have no idea what these conditions say. The condition in the outer "if" says "(this->FirstHintPosition() == nullptr) == (other->FirstHintPosition() == nullptr)", so "this->FirstHintPosition() != nullptr && other->FirstHintPosition() == nullptr" will be always false, no? https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:738: other->FirstHintPosition() != nullptr) Also seems to be always false, which means that all this newly introduced block is a no-op? In any case, such complicated conditions need some explanation in comments. https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:937: DetachAt(start, &splinter_temp, zone, true); Here it is not really clear what "true" refers to. Could you introduce an enum for readability? https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:2435: : range->NextUsePositionRegisterIsBeneficial(next_pos); Does this make any measurable difference? https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:2840: // it will result in a move. For splinters, our goal is to minimize I do not understand this reasoning - if there is no hint, then I do not see why we would have to insert a move. Could you explain? https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:2846: // we What's up with the formatting of the comments in the CL?
https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:735: other->FirstHintPosition() == nullptr) On 2016/09/29 14:20:35, Jarin wrote: > I have no idea what these conditions say. > > The condition in the outer "if" says "(this->FirstHintPosition() == nullptr) == > (other->FirstHintPosition() == nullptr)", so "this->FirstHintPosition() != > nullptr && other->FirstHintPosition() == nullptr" will be always false, no? Ya, they make no sense as written. Fixed. https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:738: other->FirstHintPosition() != nullptr) On 2016/09/29 14:20:35, Jarin wrote: > Also seems to be always false, which means that all this newly introduced block > is a no-op? > > In any case, such complicated conditions need some explanation in comments. Acknowledged. https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:937: DetachAt(start, &splinter_temp, zone, true); On 2016/09/29 14:20:35, Jarin wrote: > Here it is not really clear what "true" refers to. Could you introduce an enum > for readability? Done. https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:2435: : range->NextUsePositionRegisterIsBeneficial(next_pos); On 2016/09/29 14:20:35, Jarin wrote: > Does this make any measurable difference? I remember seeing a case where this helped avoid a split and therefore a move. https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:2840: // it will result in a move. For splinters, our goal is to minimize On 2016/09/29 14:20:35, Jarin wrote: > I do not understand this reasoning - if there is no hint, then I do not see why > we would have to insert a move. Could you explain? The hint says, for splinters, what operand the previous range segment had (the parent one). So if we can meet the hint, we don't need to move. https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:2846: // we On 2016/09/29 14:20:35, Jarin wrote: > What's up with the formatting of the comments in the CL? probably git cl format :) I'll re-brush them.
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.
I guess my general feedback is that I would be reluctant to land this without numbers justifying the extra complexity (I can already see this will cause a lot of head scratching when reading this code later). Do you know which change improved what performance numbers? https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:2840: // it will result in a move. For splinters, our goal is to minimize On 2016/09/30 05:16:51, Mircea Trofin wrote: > On 2016/09/29 14:20:35, Jarin wrote: > > I do not understand this reasoning - if there is no hint, then I do not see > why > > we would have to insert a move. Could you explain? > > The hint says, for splinters, what operand the previous range segment had (the > parent one). So if we can meet the hint, we don't need to move. I still do not really understand when we get here. How do you even know that there was any hinted position when we get here? Are you sure that not meeting hint automatically means inserting move (could not it be that we just have to use less efficient instruction encoding or some such?) https://codereview.chromium.org/2347563004/diff/100001/src/compiler/register-... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2347563004/diff/100001/src/compiler/register-... src/compiler/register-allocator.cc:737: return true; Braces. https://codereview.chromium.org/2347563004/diff/100001/src/compiler/register-... src/compiler/register-allocator.cc:740: return false; Braces
On 2016/09/30 13:23:04, Jarin wrote: > I guess my general feedback is that I would be reluctant to land this without > numbers justifying the extra complexity (I can already see this will cause a lot > of head scratching when reading this code later). > > Do you know which change improved what performance numbers? > > https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... > File src/compiler/register-allocator.cc (right): > > https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... > src/compiler/register-allocator.cc:2840: // it will result in a move. For > splinters, our goal is to minimize > On 2016/09/30 05:16:51, Mircea Trofin wrote: > > On 2016/09/29 14:20:35, Jarin wrote: > > > I do not understand this reasoning - if there is no hint, then I do not see > > why > > > we would have to insert a move. Could you explain? > > > > The hint says, for splinters, what operand the previous range segment had (the > > parent one). So if we can meet the hint, we don't need to move. > > I still do not really understand when we get here. How do you even know that > there was any hinted position when we get here? Are you sure that not meeting > hint automatically means inserting move (could not it be that we just have to > use less efficient instruction encoding or some such?) > > https://codereview.chromium.org/2347563004/diff/100001/src/compiler/register-... > File src/compiler/register-allocator.cc (right): > > https://codereview.chromium.org/2347563004/diff/100001/src/compiler/register-... > src/compiler/register-allocator.cc:737: return true; > Braces. > > https://codereview.chromium.org/2347563004/diff/100001/src/compiler/register-... > src/compiler/register-allocator.cc:740: return false; > Braces For numbers, if you take a look at patch#5, the large wasm codebases show a few percent improvement in code size. Unfortunately, since then, the 0xC update to wasm landed, and it broke some of these larger benchmarks, so I can't redo those, and we don't measure codesize otherwise (afaik).
https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2347563004/diff/80001/src/compiler/register-a... src/compiler/register-allocator.cc:2840: // it will result in a move. For splinters, our goal is to minimize On 2016/09/30 13:23:03, Jarin wrote: > On 2016/09/30 05:16:51, Mircea Trofin wrote: > > On 2016/09/29 14:20:35, Jarin wrote: > > > I do not understand this reasoning - if there is no hint, then I do not see > > why > > > we would have to insert a move. Could you explain? > > > > The hint says, for splinters, what operand the previous range segment had (the > > parent one). So if we can meet the hint, we don't need to move. > > I still do not really understand when we get here. How do you even know that > there was any hinted position when we get here? Are you sure that not meeting > hint automatically means inserting move (could not it be that we just have to > use less efficient instruction encoding or some such?) The hints for splinters are added when we create them, and they are the last operand that brings the value into the splinter interval. The splinter range itself is interrupted by intervals where the range is not deferred, so whenever a new deferred interval is added to the splinter range, its top gets hint-linked this way. Since linear scan works top-down, we would have already allocated the segments above the splinter. If we don't match the hint, we'll need somehow to transfer the value of the vreg, so that's where the move comes from. https://codereview.chromium.org/2347563004/diff/100001/src/compiler/register-... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2347563004/diff/100001/src/compiler/register-... src/compiler/register-allocator.cc:737: return true; On 2016/09/30 13:23:03, Jarin wrote: > Braces. Acknowledged. https://codereview.chromium.org/2347563004/diff/100001/src/compiler/register-... src/compiler/register-allocator.cc:740: return false; On 2016/09/30 13:23:03, Jarin wrote: > Braces Acknowledged.
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: Try jobs failed on following builders: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/13974)
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: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
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 #8 (id:140001) has been deleted
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/2347563004/diff/160001/src/compiler/register-... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2347563004/diff/160001/src/compiler/register-... src/compiler/register-allocator.cc:2832: // The goal is to reduce the number of moves, so if the hint isn't I still think this is super confusing because you are speaking of "the hint" as if it was clear that the splinter creation hint is the hint that we have here. You actually might see a completely different hint here (e.g., fixed register use hint) and spill because of that. Even worse, there might be no hint at all for the range and you would still get here and spill uselessly. This needs to be explained here because people reading this or the generated code will be puzzled why we spill randomly. I am wondering how much this particular special casing help in terms of numbers?
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.
https://codereview.chromium.org/2347563004/diff/160001/src/compiler/register-... File src/compiler/register-allocator.cc (right): https://codereview.chromium.org/2347563004/diff/160001/src/compiler/register-... src/compiler/register-allocator.cc:2832: // The goal is to reduce the number of moves, so if the hint isn't On 2016/10/07 11:10:53, Jarin wrote: > I still think this is super confusing because you are speaking of "the hint" as > if it was clear that the splinter creation hint is the hint that we have here. > You actually might see a completely different hint here (e.g., fixed register > use hint) and spill because of that. Rewrote, separately analyzing each hint kind. > > Even worse, there might be no hint at all for the range and you would still get > here and spill uselessly. > Good point, and measurements indicate we shouldn't, so addressed that. > This needs to be explained here because people reading this or the generated > code will be puzzled why we spill randomly. > > I am wondering how much this particular special casing help in terms of numbers? Measurements show that a good chunk of improvement comes from spilling.
lgtm
The CQ bit was checked by mtrofin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Avoid large deopt blocks Treat allocation of splintered ranges differently, by optimizing for move counts (i.e. try to have less move counts), rather than optimizing for quality of moves (which is what normal allocation does). We can see reductions in code size in the benchmarks that measure it (e.g. Unity) BUG= ========== to ========== [turbofan] Avoid large deopt blocks Treat allocation of splintered ranges differently, by optimizing for move counts (i.e. try to have less move counts), rather than optimizing for quality of moves (which is what normal allocation does). We can see reductions in code size in the benchmarks that measure it (e.g. Unity) BUG= ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Avoid large deopt blocks Treat allocation of splintered ranges differently, by optimizing for move counts (i.e. try to have less move counts), rather than optimizing for quality of moves (which is what normal allocation does). We can see reductions in code size in the benchmarks that measure it (e.g. Unity) BUG= ========== to ========== [turbofan] Avoid large deopt blocks Treat allocation of splintered ranges differently, by optimizing for move counts (i.e. try to have less move counts), rather than optimizing for quality of moves (which is what normal allocation does). We can see reductions in code size in the benchmarks that measure it (e.g. Unity) BUG= Committed: https://crrev.com/33629651584b669c0bd24a9be7bad868f01ea809 Cr-Commit-Position: refs/heads/master@{#40178} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/33629651584b669c0bd24a9be7bad868f01ea809 Cr-Commit-Position: refs/heads/master@{#40178} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
