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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi @lkail ,

Thanks for your patience.
This goes in the right direction.

I think we miss a few comments and some cleanups (clang-format, remove stall comments, use proper LLVM_DEBUG macros, etc.) and we're good to go!

Cheers,
-Quentin



================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:921
 
+void MachineCopyPropagation::EliminateSpillageCopies(MachineBasicBlock &MBB) {
+  LLVM_DEBUG(dbgs() << "MCP: Eliminating spillage copies in " << MBB.getName()
----------------
Add comments.
What this does: explain the algorithm at a high level.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:929
+    return Copy->getOperand(0).isRenamable() &&
+           Copy->getOperand(1).isRenamable();
+  };
----------------
Not for this patch, but you may want to use `isCopyInstr` instead of `isCopy` to catch more cases.
That said, it probably won't make much of a difference, since in particular most copies you're trying to remove here comes from splitting in regalloc (i.e., we'll have plain `COPY`).


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:929
+    return Copy->getOperand(0).isRenamable() &&
+           Copy->getOperand(1).isRenamable();
+  };
----------------
qcolombet wrote:
> Not for this patch, but you may want to use `isCopyInstr` instead of `isCopy` to catch more cases.
> That said, it probably won't make much of a difference, since in particular most copies you're trying to remove here comes from splitting in regalloc (i.e., we'll have plain `COPY`).
To be on the safe side, you may want to check that the operation has no implicit operand.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:936
+    // We don't need this chain anymore.
+    SpillChains.erase(LeadReg);
+    // Clear LeadReg.
----------------
Instead of copying the chain, can we hold on to the reference until we're done with the processing?


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:943
+    const size_t Len = Chain.size();
+    assert(Len % 2 == 0 && "Must be paired");
+
----------------
Maybe worth splitting the reload from the spill chain as this assert is strange at first glance.
Perhaps it wouldn't be as problematic when the method is properly documented.

Put differently, let's leave it like this for now, add a bunch of comments and we'll see if it still feels weird after that.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:946
+    // Bailout if less then 3 pairs.
+    if (Len < 6)
+      return;
----------------
We'll need to expand on that commet because naively, a 2 pairs chain would be beneficial to remove, but this is not something this code can do since we don't recolor outside of the spill chain.

I'd suggest putting something like: We need at least 3 pairs of copies for the transformation to apply, because the first outermost pair cannot be removed since we don't recolor outside of the chain and that we need at least one temporary spill slot to shorten the chain.
If we only have a chain of two pairs, we already have the shortest sequence this code can handle: the outermost pair for the temporary spill slot, and the pair that use that temporary spill slot for the other end of the chain.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:958
+      // PrevReloadMI->dump();
+      // ReloadMI->dump();
+      assert(PrevSpillMI->getOperand(0).getReg().asMCReg() ==
----------------
Here and other places where you have "debug" statements:
Put this in `LLVM_DEBUG` macros.
Or remove completely.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:963
+             ReloadMI->getOperand(0).getReg().asMCReg());
+    }
+    // If one in the chain is not foldable, give up.
----------------
Move that into its own helper function and call it from an assert.
```
static bool LLVM_ATTRIBUTE_UNUSED isValidChain(const SmallVectorImpl<MachineInstr *> &Chain) {
  // your checks here.
}
...
assert(isValidChain(Chain) && "Invalid chain to process");
```


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:968
+      MachineInstr *ReloadMI = Chain[I + 1];
+      if (!IsFoldableCopy(SpillMI) || !IsFoldableCopy(ReloadMI))
+        return;
----------------
I feel that this is a bit late to check that.
We should not put a copy in the "candidate" chains if the copy is not foldable.

I would suggest to handle that in the main loop.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:975
+    Chain[0]->getOperand(0).setReg(Chain[Len - 4]->getOperand(0).getReg());
+    Chain[1]->getOperand(1).setReg(Chain[Len - 3]->getOperand(1).getReg());
+    for (size_t I = 2; I != Len - 2; ++I)
----------------
By construction `Chain[Len - 4]->getOperand(0) == Chain[Len - 3]->getOperand(1)`, so I would instead put that in a variable and use it in both places.

E.g., something like:
```
// Pull the last spill slot used only within the chain as the final spill slot.
MCRegister LastReusableRegSpillSlot = Chain[Len - 4]->getOperand(0).getReg()
// Update the chain to skip all the intermediate register spill slots:
// Spilling:
Chain[0]->getOperand(0).setReg(LastReusableRegSpillSlot);
// Reload:
Chain[1]->getOperand(1).setReg(LastReusableRegSpillSlot);
```


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:977
+    for (size_t I = 2; I != Len - 2; ++I)
+      MaybeDeadCopies.insert(Chain[I]);
+  };
----------------
Maybe be worth adding a comment here that although the variable is called `MaybeDeadCopies`, we really are going to remove the related instructions.

The fact that we use `MaybeDeadCopies` to do our code cleanup is slightly confusing because if we don't actually delete the intermediate copies (a.k.a. what remains of the chain at this point) the resulting code would be incorrect.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1042
+        continue;
+      MachineInstr *MaybeCopy = Tracker.findAvailCopy(MI, Reg, *TRI);
+      if (!MaybeCopy)
----------------
At first it is strange to see that we look for a copy when `Reg` is a def, but I guess it makes sense because:
1. We are not going to recolor `Reg`
2. We need to consider this chain before it gets clobbered later in that same loop

Assuming I understood that correctly, it deserves its comment here.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1050
+      if (LeadRegs.count(SrcReg)) {
+        MCRegister LeadReg = LeadRegs[SrcReg];
+        if (SpillChains.count(LeadReg)) {
----------------
Use `LeadRegs.find` and avoid the double lookups (one in `count` and one in `operator[]`).


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1060
+      // dbgs() << "Clobbering " << printReg(Reg, TRI) << "\n";
+      Tracker.clobberRegister(Reg, *TRI);
+    }
----------------
I think this statement deserves its own comment.
IIUC here we unconditionally clobber all the registers (as opposed to only clobbering the definitions) because we only rewrite the chain itself (i.e., we don't attempt to rewrite uses after the chain).

BTW, you need to take into account regmasks too.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1060
+      // dbgs() << "Clobbering " << printReg(Reg, TRI) << "\n";
+      Tracker.clobberRegister(Reg, *TRI);
+    }
----------------
qcolombet wrote:
> I think this statement deserves its own comment.
> IIUC here we unconditionally clobber all the registers (as opposed to only clobbering the definitions) because we only rewrite the chain itself (i.e., we don't attempt to rewrite uses after the chain).
> 
> BTW, you need to take into account regmasks too.
Shouldn't we clear the `SpillChains` here for defs and not-preversed-by-regmasks regs at this point?


================
Comment at: llvm/test/CodeGen/PowerPC/mcp-elim-eviction-chain.mir:134
+
+...
----------------
Add a test with regmasks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122118