[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:196
     for (MCRegUnitIterator RUI(Src, &TRI); RUI.isValid(); ++RUI) {
-      auto I = Copies.insert({*RUI, {nullptr, {}, false}});
+      auto I = Copies.insert({*RUI, {nullptr, nullptr, {}, false}});
       auto &Copy = I.first->second;
----------------
lkail wrote:
> qcolombet wrote:
> > Use `MI` directly here instead of adding it line 200.
> Correct me if I'm woring, `Copies.insert` insert successfully only when the key doesn't exist before. Suppose we have
> ```
> L0: R0 = COPY R1
> L1: R2 = COPY R1
> ```
> `LastUseSeenInCopy` should track MI in `L1` rather than `L0`.
Ah good point!


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:309
+        if (MO.isRegMask())
+          if (MO.clobbersPhysReg(Def))
+            return nullptr;
----------------
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 :)).


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1051
+  for (size_t I = 0, S = SC.size(); I < S; ++I)
+    SC[S - 1 - I]->dump();
+  for (size_t I = 0, S = RC.size(); I < S; ++I)
----------------
qcolombet wrote:
> I think you can simplify this with a range based loop with `rbegin`/`rend`.
You should be able to use an even more compact form:
```
for (auto I : make_range(SC.rbegin(), SC.rend())
```


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1071
+// r4 = COPY r1
+// r1 = COPY r0
+// The algorithm is trying to keep
----------------
lkail wrote:
> qcolombet wrote:
> > Technically we could collapse this sequence to:
> > ```
> > // r0 = COPY r4
> > // <def-use r4>
> > // r4 = COPY r0
> > ```
> > 
> > I.e., I think it is worth explaining that the propagation doesn't check whether or not `r0` can be altered outside of the chain and that's why we conservatively keep its value as it was before the rewrite. (Would be a nice follow-up fix BTW :)).
> Maybe we can check if `r0` is killed to remove one more COPY.
Yep, but for that to be accurate we would need to flip the direction of the analysis (from top-down, to bottom-up) to get proper liveness construction.
(Or use the kill flag, but generally speaking we try to avoid relying on this.)


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1092
+void MachineCopyPropagation::EliminateSpillageCopies(MachineBasicBlock &MBB) {
+  // FIXME: Not sure why using DenseMap hit assertion inside DenseMap.
+  std::map<MachineInstr *, MachineInstr *> ChainLeader;
----------------
lkail wrote:
> qcolombet wrote:
> > That sounds weird.
> > Would you mind sharing the assertion?
> It hits `DenseMapIterator`'s
> ```
>   pointer operator->() const {
>     assert(isHandleInSync() && "invalid iterator access!");
> ...
>   }
> ```
> I dived into it a bit, looks it's checking the validity of the iterator, i.e., if the container is updated, the iterator constructed before the update is invalid.
> Code like
> ```
>         auto Leader = ChainLeader.find(MaybePrevReload);
> ...
>         ChainLeader.insert({MaybeSpill, Leader->second});
>         ChainLeader.insert({&MI, Leader->second});
> ```
> Should be avoided.
Yep, every time you insert something in the dense map, you may invalidate the iterators.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1094
+  std::map<MachineInstr *, MachineInstr *> ChainLeader;
+  DenseMap<MachineInstr *, SmallVector<MachineInstr *>> SpillChain, ReloadChain;
+  // If a COPY's Source has use or def untill next COPY defines the Source,
----------------
lkail wrote:
> qcolombet wrote:
> > Could you add a comment on what the mappings hold (all three of them)?
> > 
> > I haven't read the code past this point yet, but for instance I would have expected that the key in these maps would be a `Register` not `MachineInstr`.
> > I haven't read the code past this point yet, but for instance I would have expected that the key in these maps would be a Register not MachineInstr.
> 
> I separate the algorithm implementation in to two stages. stage1: Collect spill-reload chains. stage2: Fold the chains. If using `Register`, we are unable to track different spill-reload chains that share same registers.
I see, make sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122118