[PATCH] D98659: [MachineCopyPropagation] Do more backward copy propagations

[yshui via Phabricator via llvm-commits]

yshui added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:160-162
+  /// Mapping registers to where they are used later. Key is actual registers
+  /// (not register units) Only used for Backward Propagation
+  DenseMap<MCRegister, SmallVector<MachineInstr *>> Uses;
----------------
jmorse wrote:
> The key is only MCRegisters, but invalidateRegister and similar functions only work with register units. How are Uses found so that they can be invalidated too?
`Uses` is always accessed with registers as key (instead of register units), whether it's to find entries or to invalidate entries.

Unless I misunderstood your question?


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:198-199
+
+    // Make sure when we remove Src registers from Copies, we remove the entire
+    // register used in the copy instruction.
+    for (MCRegister InvalidDefReg : RegsToInvalidate) {
----------------
jmorse wrote:
> Could you explain how this is different from the original code -- after all, the original implementation invalidated copes for the reguints for source and destinations of copies found.  I don't quite understand "entire register" -- is it the set of regunits, the aliases, or something else?
So entries of `Copies` form pairs. Say we have a `Def = COPY Src`, then `Copies` will contain `{RegUnits of Def}` and `{RegUnits of Src}` as a pair. `trackUse` relies on the invariant that when `Def = COPY Src` is no longer a candidate, //all// related entries in `Copies` are removed.

The original code doesn't guarantee this. When a RegUnit of a Src register is passed to the original `invalidateRegister`, only that RegUnit, as well as the related Def RegUnits will be removed, leaving all other RegUnits of that Src register around. 

This change makes sure all the related RegUnits are removed.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:388-392
+    for (MCRegister InvalidReg : RegsToInvalidate) {
+      for (MCRegUnitIterator RUI(InvalidReg, &TRI); RUI.isValid(); ++RUI)
+        Copies.erase(*RUI);
+      Uses.erase(InvalidReg);
+    }
----------------
jmorse wrote:
> Shouldn't this be left to the rest of the machine-copy-propagate implementation -- after all, this method is called "trackUse", and it seems to be doing more than that.
`trackUse` tracks the uses that are rewrite candidates, so if a use is no longer a candidate for rewrite, `trackUse` removes it.

I think this makes sense.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:935
+        if (User->isDebugValue()) {
+          MRI->updateDbgUsersToReg(Def, User);
+        } else {
----------------
jmorse wrote:
> I ran a stage2 RelWithDebInfo build of clang to test this (I was curious), and got an assertion failure in this function:
> 
>   llvm::MachineRegisterInfo::updateDbgUsersToReg(llvm::Register, ArrayRef<llvm::MachineInstr *>) const: Assertion `MI->getOperand(0).isReg()' failed.
> 
Interesting. I assume this is on X86? Do you happen to have the offending MIR around?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98659