[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: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;
----------------
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`.


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


================
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;
----------------
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.


================
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,
----------------
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.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1097
+  // we put the COPY in this set to keep property#2.
+  DenseSet<MachineInstr *> CopySourceInvalid;
+
----------------
qcolombet wrote:
> Instead of tracking that, should we just invalidate the chain / stop it before that point?
The implementation doesn't invalidate any chain in stage1. Compared to previous implementation, I think current one is easier to reason and easier to maintain. When we are iterating MI inside the MBB, we don't know which `COPY` might be one of the innermost spill-reload pair and we don't want to lose track of the innermost spill-reload pair. The Source of the innermost spill is allowed to be re-use and re-def between the innermost spill-reload pair.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122118