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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:309
+        if (MO.isRegMask())
+          if (MO.clobbersPhysReg(Def))
+            return nullptr;
----------------
lkail wrote:
> 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.
> Ah, I see. Is it a good idea to enumerate them via TRI->getNumRegs() and check RegMask to see if they are preserve/clobber?

That's the idea. Though we wouldn't need to enumerate all the registers, only the ones that you care about.
What you're doing here is already fine.

The thing that bothers me in the current code is the call to `clobberRegister`. This modifies the state of the tracker. Usually the `find` methods only query the `RegMask`s directly (with `MachineOperand::clobbersPhysReg`).

> Correct me if I still fail to get your point.

You got the point.
Let's keep the code as is for now and let me do a full pass on the code so that I have a better model of how the whole things works.

I'll probably won't have time to do it before next week though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122118