[PATCH] D122118: [MachineCopyPropagation] Eliminate spillage copies that might be caused by eviction chain

[Kai Luo via Phabricator via llvm-commits]

lkail added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:309
+        if (MO.isRegMask())
+          if (MO.clobbersPhysReg(Def))
+            return nullptr;
----------------
qcolombet wrote:
> lkail wrote:
> > qcolombet wrote:
> > > If `Def` is clobbered between `DefCopy` and `Current` I would have expected that `DefCopy` would have been removed from `Copies`.
> > > 
> > > Put differently, I feel that if we have to check that here, the `Copies` map is holding hard-to-reason-about information.
> > > 
> > > Are we missing some call to `clobberRegister`?
> > I can imagine there might be some `RegMask`s doesn't implicit def any registers, just clobber them. When we are traversing the MBB and are encountered with a `RegMask` without any other implicit-def, we don't know which register to clobber directly. I'm not sure we have a way to enumerate registers a `RegMask` clobbers. If there is such a way, I think we should clobber registers when we are traversing the MBB, not checking `RegMask` clobbers here.
> I think I see our misunderstanding.
> Given the name of the function I would expect that this function only does some queries on the tracker, but you're actually using this function to do some bookkeeping as well.
> 
> So the conclusion is either rename this function to more accurately represents what it does (I don't have a good name for now) or move the bookkeeping in the main loop (i.e., I thought we were calling clobberRegister from the main loop already.)
> 
> Regarding your comment on `RegMask`s, I am not sure I follow:
> - `RegMask`s always list all the registers they preserve/clobber
> - Liveness sets at basic block boundaries are not represented with `RegMask`s, but anyway we don't care because the tracking is always purely local to a basic block in that pass. (Unless you've changed that and I missed it :)).
> RegMasks always list all the registers they preserve/clobber

Ah, I see. Is it a good idea to enumerate them via `TRI->getNumRegs()` and check `RegMask` to see if they are preserve/clobber?

For the origin question

> If Def is clobbered between DefCopy and Current I would have expected that DefCopy would have been removed from Copies.

Does it imply we should not check `RegMask` and don't update bookkeeping by calling `Tracker::clobberRegister` here, checking if `RegMask` clobbers registers should already have been conducted in the main loop?

Correct me if I still fail to get your point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122118