[PATCH] D55295: LiveIntervals: Add removePhysReg

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added inline comments.
This revision is now accepted and ready to land.


================
Comment at: include/llvm/CodeGen/LiveIntervals.h:423
+    /// Reg. Subsequent uses should rely on on-demand recomputation.
+    void removePhysReg(unsigned Reg) {
+      for (MCRegUnitIterator Units(Reg, TRI); Units.isValid(); ++Units)
----------------
arsenm wrote:
> qcolombet wrote:
> > kparzysz wrote:
> > > arsenm wrote:
> > > > kparzysz wrote:
> > > > > qcolombet wrote:
> > > > > > arsenm wrote:
> > > > > > > kparzysz wrote:
> > > > > > > > Could this be called `removePhysRegUnits`?  I think it would make it clearer what it actually does.
> > > > > > > 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
> > > > > > Agree with Matt.
> > > > > 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".
> > > > What about an overly verbose name like "removeAllRegUnitsForPhysReg"?
> > > Sure, why not. :)
> > 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.
> 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 
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55295/new/

https://reviews.llvm.org/D55295