[PATCH] D157564: [MCP] Invalidate copy for super register in copy source

[Kai Luo via Phabricator via llvm-commits]

lkail added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:141
       if (I != Copies.end()) {
-        if (MachineInstr *MI = I->second.MI) {
-          std::optional<DestSourcePair> CopyOperands =
-              isCopyInstr(*MI, TII, UseCopyInstr);
-          assert(CopyOperands && "Expect copy");
+        MachineInstr *MI = I->second.MI ? I->second.MI : I->second.LastSeenUseInCopy;
+        std::optional<DestSourcePair> CopyOperands =
----------------
```diff
@@ -137,18 +137,20 @@ public:
     // and invalidate all of them.
     SmallSet<MCRegister, 8> RegsToInvalidate;
     RegsToInvalidate.insert(Reg);
+    auto invalidate = [&](MachineInstr *MI) {
+      std::optional<DestSourcePair> CopyOperands =
+          isCopyInstr(*MI, TII, UseCopyInstr);
+      assert(CopyOperands && "Expect copy");
+      RegsToInvalidate.insert(CopyOperands->Destination->getReg().asMCReg());
+      RegsToInvalidate.insert(CopyOperands->Source->getReg().asMCReg());
+    };
     for (MCRegUnit Unit : TRI.regunits(Reg)) {
       auto I = Copies.find(Unit);
       if (I != Copies.end()) {
-        if (MachineInstr *MI = I->second.MI) {
-          std::optional<DestSourcePair> CopyOperands =
-              isCopyInstr(*MI, TII, UseCopyInstr);
-          assert(CopyOperands && "Expect copy");
-
-          RegsToInvalidate.insert(
-              CopyOperands->Destination->getReg().asMCReg());
-          RegsToInvalidate.insert(CopyOperands->Source->getReg().asMCReg());
-        }
+        if (MachineInstr *MI = I->second.MI)
+          invalidate(MI);
+        if (MachineInstr *MI = I->second.LastSeenUseInCopy)
+          invalidate(MI);
         RegsToInvalidate.insert(I->second.DefRegs.begin(),
                                 I->second.DefRegs.end());
       }
```
We should check both of `I->second.MI` and `I->second.LastSeenUseInCopy`.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:191-192
 
     MCRegister Src = CopyOperands->Source->getReg().asMCReg();
     MCRegister Def = CopyOperands->Destination->getReg().asMCReg();
 
----------------
jrbyrnes wrote:
> arsenm wrote:
> > Can we track all of this in regunits? Following registers is way more confusing
> There are many places that use register MCRegister equality / `isSubRegisterEq` on CopyOperands->Destination / Source. In order to prove equality for regunits, we would need to iteratively do set equality which seems like it would make things more complicated. Maybe I've missed something?
I agree with that we should track at regunit basis. Maybe we can do that in another NFC patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157564