Page MenuHomePhabricator

[RegisterCoalescer] More fixes for subreg join failure in RegCoalescer
ClosedPublic

Authored by dstuttard on Nov 21 2017, 7:54 AM.

Details

Summary

Part of the adjustCopiesBackFrom method wasn't correctly dealing with SubRange
intervals when updating.

2 changes. The first to ensure that bogus SubRange Segments aren't propagated when
encountering Segments of the form [1234r, 1234d:0) when preparing to merge value
numbers. These can be removed in this case.

The second forces a shrinkToUses call if SubRanges end on the copy index
(instead of just the parent register).

The included test is a cutdown reproducer in llvm-ir form rather than mir as the
mir variant doesn't provoke the bug.

Diff Detail

Repository
rL LLVM

Event Timeline

dstuttard created this revision.Nov 21 2017, 7:54 AM

Any comment on this change? If not I'll land in the next few days.

Hi David,

If not I'll land in the next few days.

Please don't do that. When a patch is up for review you're basically need someone to look at it before pushing it.

As for the patch itself, it looks reasonable, but before going further I'd like we understand why the .mir test doesn't reproduce it.
I am suspecting that what you're fixing is a somewhat invalid from of live-ranges that has been introduced by a bug in some previous updates of the liveness information.

Could you go to the bottom of this?

Cheers,
-Quentin

lib/CodeGen/RegisterCoalescer.cpp
592 ↗(On Diff #123806)

Period at the end of comments.

MatzeB accepted this revision.Feb 15 2018, 11:46 AM

LGTM, with same change to avoid unnecessary repeated shrinkToUses().

lib/CodeGen/RegisterCoalescer.cpp
620–625 ↗(On Diff #123806)

You only ever need to do shrinkToUses once as it will deal with all subranges. Therefore I'd propose to change this block to:

// Rewrite the copy.
CopyMI->substituteRegister(IntA.reg, IntB.reg, 0, *TRI);
// If the copy instruction was killing the destination register or any
// subrange before the merge trim the live range.
bool RecomputeLiveRange = AS->end == CopyIdx;
if (!RecomputeLiveRange) {
  for (LiveInterval::SubRange &S : IntA.subranges()) {
    LiveInterval::iterator SS = S.FindSegmentContaining(CopyUseIdx);
    if (SS != S.end() && SS->end == CopyIdx) {
      RecomputeLiveRange = true;
      break;
    }
  }
}
if (RecomputeLiveRange)
  shrinkToUses(&IntA);
This revision is now accepted and ready to land.Feb 15 2018, 11:46 AM
tpr added a subscriber: tpr.Jul 9 2018, 9:14 AM

I think this got superseded by D48102.

tpr added a comment.Jul 16 2018, 11:06 AM

Actually this is not superseded by D48102.

This revision was automatically updated to reflect the committed changes.