Page MenuHomePhabricator

LiveIntervalAnalysis: Avoid multiple connected liveness components
ClosedPublic

Authored by MatzeB on Sep 21 2015, 4:45 PM.

Details

Summary

We may have subregister defs which are unused but not discovered and
cleaned up prior to liveness analysis. This creates multiple connected
components in the resultsing live range which are forbidden in the
MachineVerifier because they would unnecesarily constrain the register
allocation. Rewrite those dead definitions to define a newly created
virtual register.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 35320.Sep 21 2015, 4:45 PM
MatzeB retitled this revision from to LiveIntervalAnalysis: Avoid multiple connected liveness components.
MatzeB updated this object.
MatzeB added a reviewer: qcolombet.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added subscribers: llvm-commits, arsenm.
qcolombet edited edge metadata.Sep 21 2015, 5:28 PM

Hi Matthias,

Nice catch!

Right now, this is on the users of shrinkUses to fix the multi component splitting.
The rationale, I think :), is that we want the users to be aware of the any virtual registers that would be created. This is not the case in that patch.

The bottom line is I believe shrinkToUses should return true in those situations and we should fix the users of shrinkToUses, if need to.
Indeed, I, for instance, guess that the register coalescer would already do the right thing if we return true, since it creates new virtual registers, and wouldn’t have to fix it.

What do you think?

Cheers,
-Quentin

Hi Matthias,

Nice catch!

Right now, this is on the users of shrinkUses to fix the multi component splitting.
The rationale, I think :), is that we want the users to be aware of the any virtual registers that would be created. This is not the case in that patch.

The bottom line is I believe shrinkToUses should return true in those situations and we should fix the users of shrinkToUses, if need to.
Indeed, I, for instance, guess that the register coalescer would already do the right thing if we return true, since it creates new virtual registers, and wouldn’t have to fix it.

What do you think?

computeDeadValues() is also used during the initial Liveness computation in which the return value is not checked (because we cannot have deadPhi values at that place). Seeing on the other hand that the creation of the new VReg is trivial and in this case faster because we know exactly that we will only have a single dead definition I decided to do the LiveInterval creation inline.

I also checked all places where shrinkToUses() is called to ensure they can deal with the fact that additional LiveIntervals may have been created.

I guess if it is needed we could have an additional optional parameter (like SmallVectorImpl<unsigned> *) to return the virtual registers created.
Seems like it is not needed for now.

The thing that I do not like is that it seems to me that the handling of multi component is not homogeneous between the non-subregister and the subregister case.

computeDeadValues() is also used during the initial Liveness computation in which the return value is not checked (because we cannot have deadPhi values at that place).

How hard would it be to account for the returned value?

Thanks,
-Quentin

MatzeB updated this revision to Diff 35427.Sep 22 2015, 3:16 PM
MatzeB edited edge metadata.

New version which uses the existing mechanisms to split the live interval into multiple components.

qcolombet accepted this revision.Sep 22 2015, 3:27 PM
qcolombet edited edge metadata.

LGTM.

Cheers,
-Quentin

This revision is now accepted and ready to land.Sep 22 2015, 3:27 PM
This revision was automatically updated to reflect the committed changes.