This is an archive of the discontinued LLVM Phabricator instance.

RegAllocGreedy: Roll back successful recolorings on failure
ClosedPublic

Authored by arsenm on Mar 28 2022, 6:12 AM.

Details

Summary

This is a replacement for the original fix attempted in
c46aab01c002b7a04135b8b7f1f52d8c9ae23a58.

This fixes "overlapping insert" assertion failures when trying to
unwind an unsuccessful recoloring attempt.

The problem would occur when there are multiple recoloring candidates
which recursively required recoloring. If one recoloring candidate was
successfully recolored at one level, and the next recoloring candidate
was unsuccessful, we would not roll back the first candidates
successful recoloring. The forgotten successful recoloring may have
been assigned to something that conflicts with a register that needs
to be restored in a parent recoloring attempt.

See the testcase added in issue48473 for a more concrete example with
explanation.

Diff Detail

Event Timeline

arsenm created this revision.Mar 28 2022, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 6:12 AM
arsenm requested review of this revision.Mar 28 2022, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 6:12 AM
arsenm updated this revision to Diff 418564.Mar 28 2022, 6:16 AM

Restore accidentally removed assert

foad added a subscriber: foad.Mar 28 2022, 6:16 AM

Comments on the RISC-V test to keep it consistent with all the others

llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
2

Use just -mtriple=riscv64

10

Call it something other than main?

100–105

Don't need any of these (target-features should be in llc's mattr command line

arsenm updated this revision to Diff 418609.Mar 28 2022, 9:10 AM

Minimize target features

jrtc27 added inline comments.Mar 28 2022, 9:44 AM
llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
90

Is this really what you want rather than ret void?

95

Give this a more reasonable name, separate it from the intrinsics and drop the unnecessary attributes (dso_local and local_unnamed_addr do nothing useful for declarations on RISC-V)

102–105

These shouldn't be needed

arsenm added inline comments.Mar 28 2022, 9:48 AM
llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
90

It doesn't really matter, this was just the reported reduced testcase

102–105

These are part of the intrinsic declarations and aren't adding additional information. I prefer to keep attributes that are hardcoded on the intrinsic explicitly

jrtc27 added inline comments.Mar 28 2022, 10:11 AM
llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
102–105

We don't in RISC-V tests, they're not necessary and just add clutter

jrtc27 added inline comments.Mar 28 2022, 10:12 AM
llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
90

Sure, bugpoint is a bit overly aggressive. You'll probably also have certain people shouting at you for using undef rather than poison everywhere above...

arsenm updated this revision to Diff 418706.Mar 28 2022, 2:23 PM

Test changes

qcolombet accepted this revision.Mar 29 2022, 12:11 PM

Thanks @arsenm for tracking this down and the fix!

LGTM.

This revision is now accepted and ready to land.Mar 29 2022, 12:11 PM

I've run some tests with this patch over night without problems. We end up in tryLastChanceRecoloring fairly often when running tests on our out-of-tree target so I have probably exercised the new code a bit at least.