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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi @lkail,

I am halfway through.

I'm sharing my comments so far if you want to get started with some of the nitpicks.

Cheers,
-Quentin



================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:106
   struct CopyInfo {
-    MachineInstr *MI;
+    MachineInstr *MI, *LastUseCopy;
     SmallVector<MCRegister, 4> DefRegs;
----------------
Maybe rename in `LastSeenUseInCopy`.

Essentially, I would avoid `LastUse` alone as it carries a lot of expected semantic that I don't think apply here.


================
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;
----------------
Use `MI` directly here instead of adding it line 200.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:290
+  // Find last COPY that defines Reg before Current MachineInstr.
+  MachineInstr *findLastDefCopy(MachineInstr &Current, MCRegister Reg,
+                                const TargetRegisterInfo &TRI,
----------------
Could `Current` be const here?


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


================
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)
----------------
I think you can simplify this with a range based loop with `rbegin`/`rend`.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1052
+    SC[S - 1 - I]->dump();
+  for (size_t I = 0, S = RC.size(); I < S; ++I)
+    RC[I]->dump();
----------------
That should be a simple range based loop:
```
for (const MachineInstr *MI: RC)
  MI->dump();
``` 


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1071
+// r4 = COPY r1
+// r1 = COPY r0
+// The algorithm is trying to keep
----------------
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 :)).


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1074
+// property#1: No Def of spill COPY in the chain is used or defined untill the
+// paired reload COPY in the chainuses the Def.
+//
----------------
Typo: until


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1074
+// property#1: No Def of spill COPY in the chain is used or defined untill the
+// paired reload COPY in the chainuses the Def.
+//
----------------
qcolombet wrote:
> Typo: until
typo: chain uses


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1087
+// out last COPY that defines Reg; we use CopyTracker::findLastUseCopy(Reg, ...)
+// to find out last COPY that uses Reg. When we are encuntered with a Non-COPY
+// instruction, we check registers in the operands of this instruction. If this
----------------
typo: encountered


================
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;
----------------
That sounds weird.
Would you mind sharing the assertion?


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


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1095
+  DenseMap<MachineInstr *, SmallVector<MachineInstr *>> SpillChain, ReloadChain;
+  // If a COPY's Source has use or def untill next COPY defines the Source,
+  // we put the COPY in this set to keep property#2.
----------------
typo: until


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1097
+  // we put the COPY in this set to keep property#2.
+  DenseSet<MachineInstr *> CopySourceInvalid;
+
----------------
Instead of tracking that, should we just invalidate the chain / stop it before that point?


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1110
+    // the temporary spill slot, and the pair that use that temporary spill slot
+    // for the other end of the chain.
+    if (SC.size() <= 2)
----------------
Maybe add a todo that if the outermost pair of copies modifies a register that is dead outside of that pair, we could eliminate one more pair.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122118