Page MenuHomePhabricator

LiveIntervals: Add removePhysReg
ClosedPublic

Authored by arsenm on Dec 4 2018, 1:37 PM.

Details

Summary

Convenience wrapper for removing the reg units of a physical register.

Diff Detail

Event Timeline

arsenm created this revision.Dec 4 2018, 1:37 PM
qcolombet accepted this revision.Feb 13 2019, 10:26 AM

LGTM with nitpick.

include/llvm/CodeGen/LiveIntervals.h
422

The line break between \p and the name of the argument looks weird.

This revision is now accepted and ready to land.Feb 13 2019, 10:26 AM
kparzysz added inline comments.
include/llvm/CodeGen/LiveIntervals.h
423

Could this be called removePhysRegUnits? I think it would make it clearer what it actually does.

arsenm marked an inline comment as done.Feb 13 2019, 10:45 AM
arsenm added inline comments.
include/llvm/CodeGen/LiveIntervals.h
423

You are passing it a physreg, not a regunit so I think that would be more confusing. This is sort of hiding the implementation detail that the liveness of RegUnits is tracked

qcolombet added inline comments.Feb 13 2019, 10:51 AM
include/llvm/CodeGen/LiveIntervals.h
423

Agree with Matt.

kparzysz added inline comments.Feb 13 2019, 11:21 AM
include/llvm/CodeGen/LiveIntervals.h
423

There exist register units that are shared between physical registers, so removing all units contained in a phys reg A can affect units from phys reg B. In that sense removing register A affects information about register B. It appears counterintuitive when you think of it as "removing a register", but makes sense when you think about "removing register's units".

I'm not trying to block this from being committed, but I'm really not a fan of it. I think that register units are fundamental enough not to be treated as an "implementation detail". Besides, they are quite explicit in the public interface of LiveIntervals.

arsenm marked an inline comment as done.Feb 13 2019, 12:09 PM
arsenm added inline comments.
include/llvm/CodeGen/LiveIntervals.h
423

What about an overly verbose name like "removeAllRegUnitsForPhysReg"?

kparzysz accepted this revision.Feb 13 2019, 1:08 PM
kparzysz added inline comments.
include/llvm/CodeGen/LiveIntervals.h
423

Sure, why not. :)

qcolombet requested changes to this revision.Feb 13 2019, 1:28 PM
qcolombet added inline comments.
include/llvm/CodeGen/LiveIntervals.h
423

Good point. Actually, this makes me wonder about the usefulness of this API.
Arguably, we should still track a reg-unit if there exists at least one phys-reg that uses it and that API does not capture that.
Moreover, unless you know about the internal representation, it is pretty easy to shoot ourselves (e.g., I forgot the reg unit were shared when I agreed to the patch).

All in all, now I am not convinced we should add that.

This revision now requires changes to proceed.Feb 13 2019, 1:28 PM
arsenm marked an inline comment as done.Feb 13 2019, 6:04 PM
arsenm added inline comments.
include/llvm/CodeGen/LiveIntervals.h
423

There are a number of places that this is repeated. AMDGPU has a few places which should be doing this, but incorrectly remove only the first regunit in the iterator (in places where it's assumed the intervals will be recomputed as needed)

I also use this a few places in D55301, where the live intervals just need to be deleted since they won't be needed again.

Also for naming reference, RegScavenger::removeRegUnits

qcolombet accepted this revision.Feb 14 2019, 9:56 AM
qcolombet added inline comments.
include/llvm/CodeGen/LiveIntervals.h
423

Alright sounds good then.
Just add a \note or \warning that using this method can result in inconsistent liveness tracking (maybe with an example with two physical reg on the sharing a regunit) and should be used cautiously.

This revision is now accepted and ready to land.Feb 14 2019, 9:56 AM
arsenm closed this revision.Feb 22 2019, 11:03 AM

r354685