Page MenuHomePhabricator

[RegisterCoalescer] Extend a subrange if needed when filling range gap
ClosedPublic

Authored by kparzysz on Apr 15 2020, 3:17 PM.

Details

Reviewers
qcolombet
arsenm
Summary

Register live ranges may have had gaps that after coalescing should be removed. This is done by adding a new segment to the range, and merging it with neighboring segments. When doing so, do not assume that each subrange of the register ended at the same index. If a subrange ended earlier, adding this segment could make the live range invalid. Instead, if the subrange is not live at the start of the segment, extend it first.

Diff Detail

Event Timeline

kparzysz created this revision.Apr 15 2020, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2020, 3:17 PM
arsenm added inline comments.Apr 15 2020, 3:37 PM
llvm/test/CodeGen/Hexagon/regalloc-coal-extend-short-subrange.mir
18

Can you reduce this further?

kparzysz marked an inline comment as done.Apr 15 2020, 4:48 PM
kparzysz added inline comments.
llvm/test/CodeGen/Hexagon/regalloc-coal-extend-short-subrange.mir
18

Not by much if at all. This is what I got from a bugpoint testcase that was reduced pretty much as far as it would go.

kparzysz marked an inline comment as not done.Apr 15 2020, 4:49 PM
kparzysz marked an inline comment as done.Apr 15 2020, 5:02 PM
kparzysz added inline comments.
llvm/test/CodeGen/Hexagon/regalloc-coal-extend-short-subrange.mir
18

I just tried removing some things (replacing function arguments with undefs), but the testcase stopped failing.

arsenm added inline comments.Apr 16 2020, 1:39 PM
llvm/test/CodeGen/Hexagon/regalloc-coal-extend-short-subrange.mir
18

Reducing these kinds of thing is difficult and time consuming, but in my experience doable. One technique I've used before is dumping the MIR from the middle of the register coalescer before the final change that hits the verifier error (and a combination of manually coalescing some of the copies). If it's so fragile the test may not end up usefully stressing this situation in the future

kparzysz updated this revision to Diff 258327.Apr 17 2020, 7:51 AM

Created a smaller testcase.

Our local development teams are hitting this problem. Is this patch reasonable? Could someone take a look?

bzEq added a subscriber: bzEq.Apr 21 2020, 7:08 PM
qcolombet accepted this revision.May 4 2020, 10:02 AM
This revision is now accepted and ready to land.May 4 2020, 10:02 AM