This is an archive of the discontinued LLVM Phabricator instance.

[RegAllocGreedy] Attempt to split unspillable live intervals
ClosedPublic

Authored by dylanmckay on Sep 29 2016, 10:23 PM.

Details

Summary

Previously, when allocating unspillable live ranges, we would never
attempt to split. We would always bail out and try last ditch graph
recoloring.

This patch changes this by attempting to split all live intervals before
performing recoloring.

This fixes LLVM bug PR14879.

I can't add test cases for any backends other than AVR because none of
them have small enough register classes to trigger the bug.

Event Timeline

dylanmckay retitled this revision from to [RegAllocGreedy] Attempt to split unspillable live intervals.
dylanmckay updated this object.
dylanmckay added a reviewer: qcolombet.
This revision was automatically updated to reflect the committed changes.

This was accidentally committed and immediately reverted.

dylanmckay reopened this revision.Sep 30 2016, 7:15 AM
qcolombet edited edge metadata.Oct 5 2016, 10:54 AM
qcolombet added a subscriber: llvm-commits.

+llvm-commits

Hi,

As it is, I don't think this will do what you want.
Namely, I think when we spilt a live-range, we do not mark the new live-ranges as unspillable. Thus, I believe we may spill the new split products of an unspillable live-range.

At the very least, I would expect we walk the NewVRegs list and mark the elements as unspillable.
That being said, the proper fix is probably within LiveRangeEdit.

I let you have a look and get back to me if you have any question.

Cheers,
-Quentin

dylanmckay updated this revision to Diff 73901.Oct 7 2016, 2:56 AM
dylanmckay edited edge metadata.

Don't split an unspillable live interval into spillable live intervals

Hey Quentin,

That was a nice catch. I've updated the diff so that it no longer splits unspillable live intervals into spillable live intervals.

Hi Dylan,

I don't know if phabricator is acting strange but there is something odd with the diff.
I see things that do not seem to be part of that diff (AVRMCInstLower.*) and the changes in RegAllocGreedy.cpp disappeared.

Could you check the uploaded patch?

Thanks,
-Quentin

lib/CodeGen/LiveRangeEdit.cpp
443

No brackets for one line blocks per LLVM coding style.

dylanmckay updated this revision to Diff 74003.Oct 7 2016, 5:06 PM

Remove extraneous braces from if block.

Also fixed a rebase-gone-wrong and removed the AVRMCInstLower stuff that I recently committed to trunk.

dylanmckay marked an inline comment as done.Oct 7 2016, 5:06 PM
qcolombet accepted this revision.Oct 10 2016, 10:31 AM
qcolombet edited edge metadata.

Hi Dylan,

LGTM.

Few nitpicks inlined, no need for another round of review.

Cheers,
-Quentin

lib/CodeGen/RegAllocGreedy.cpp
2557

if (Stage < RS_Spill) should do the trick and is more understandable IMO.

test/CodeGen/AVR/high-pressure-on-ptrregs.ll
37

Also check that the generated code looks reasonable.

51

Use opt -instnamer to get rid of the [0-9]+ variables.

This revision is now accepted and ready to land.Oct 10 2016, 10:31 AM
dylanmckay updated this revision to Diff 74194.Oct 10 2016, 6:10 PM
dylanmckay marked 3 inline comments as done.
dylanmckay edited edge metadata.

Code cleanups from Quentin

  • Use the instnamer pass to clean up the testcase
  • Change condition from 'is split 1 or is split 2' to 'less than spill'
test/CodeGen/AVR/high-pressure-on-ptrregs.ll
51

Didn't know of that - nice.

dylanmckay closed this revision.Oct 10 2016, 6:14 PM

Committed in r283838