LiveIntervals: Add removePhysReg

[arsenm]

Summary

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

[arsenm]

ping

[qcolombet]

LGTM with nitpick.

include/llvm/CodeGen/LiveIntervals.h

422

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

[kparzysz]

include/llvm/CodeGen/LiveIntervals.h

423

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

[arsenm]

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]

include/llvm/CodeGen/LiveIntervals.h

423

Agree with Matt.

[kparzysz]

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".

[kparzysz]

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]

include/llvm/CodeGen/LiveIntervals.h

423

What about an overly verbose name like "removeAllRegUnitsForPhysReg"?

[kparzysz]

include/llvm/CodeGen/LiveIntervals.h

423

Sure, why not. :)

[qcolombet]

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.

[arsenm]

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]

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.

[arsenm]

r354685

include/llvm/CodeGen/LiveIntervals.h

public:
 
     /// Remove computed live range for register unit \p Unit. Subsequent uses
     /// should rely on on-demand recomputation.
     void removeRegUnit(unsigned Unit) {
       delete RegUnitRanges[Unit];
       RegUnitRanges[Unit] = nullptr;
     }
 
+    /// Remove associated live ranges for the register units associated with \p
+    /// Reg. Subsequent uses should rely on on-demand recomputation.
+    void removePhysReg(unsigned Reg) {
+      for (MCRegUnitIterator Units(Reg, TRI); Units.isValid(); ++Units)
+        removeRegUnit(*Units);
+    }
+
     /// Remove value numbers and related live segments starting at position
     /// \p Pos that are part of any liverange of physical register \p Reg or one
     /// of its subregisters.
     void removePhysRegDefAt(unsigned Reg, SlotIndex Pos);
 
     /// Remove value number and related live segments of \p LI and its subranges
     /// that start at position \p Pos.
     void removeVRegDefAt(LiveInterval &LI, SlotIndex Pos);