[MCP] Invalidate copy for super register in copy source

We must also track the super sources of a copy, otherwise we introduce a sort of subtle bug.

Consider:

1.  DEF r0:r1
2.  USE r1
3.  r6:r9 = COPY r10:r13
4.  r14:15 = COPY r0:r1
5.  USE r6
6.. r1:4 = COPY r6:9

BackwardCopyPropagateBlock processes the instructions from bottom up. After processing 6., we will have propagatable copy for r1-r4 and r6-r9. After 5., we invalidate and erase the propagatble copy for r1-r4 and r6 but not for r7-r9.

The issue is that when processing 3., data structures still say we have valid copies for dest regs r7-r9 (from 6.). The corresponding defs for these registers in 6. are r1:r4, which we mark as registers to invalidate. When invalidating, we find the copy that corresponds to r1 is 4. (this was added when processing 4.), and we say that r1 now maps to unpropagatable copies. Thus, when we process 2., we do not have a valid copy, but when we process 1. we do -- because the mapped copy for subregister r0 was never invalidated.

The net result is to propagate the copy from 4. to 1., and replace DEF r0:r1 with DEF r14:r15. Then, we have a use before def in 2.

The main issue is that we have an inconsitent state between which def regs and which src regs are valid. When processing 5., we mark all the defs in 6. as invalid, but only the subreg use as invalid. Either we must only invalidate the individual subreg for both uses and defs, or the super register for both.

Differential Revision: https://reviews.llvm.org//D157564

Change-Id: I99d5e0b1a0d735e8ea3bd7d137b6464690aa9486

Author: jrbyrnes

llvm/lib/CodeGen/MachineCopyPropagation.cpp

@@ -134,28 +134,31 @@ class CopyTracker {
                           const TargetInstrInfo &TII, bool UseCopyInstr) {
     // Since Reg might be a subreg of some registers, only invalidate Reg is not
     // enough. We have to find the COPY defines Reg or registers defined by Reg
-    // and invalidate all of them.
-    SmallSet<MCRegister, 8> RegsToInvalidate;
-    RegsToInvalidate.insert(Reg);
+    // and invalidate all of them. Similarly, we must invalidate all of the
+    // the subregisters used in the source of the COPY.
+    SmallSet<MCRegUnit, 8> RegUnitsToInvalidate;
+    auto InvalidateCopy = [&](MachineInstr *MI) {
+      std::optional<DestSourcePair> CopyOperands =
+          isCopyInstr(*MI, TII, UseCopyInstr);
+      assert(CopyOperands && "Expect copy");
+
+      auto Dest = TRI.regunits(CopyOperands->Destination->getReg().asMCReg());
+      auto Src = TRI.regunits(CopyOperands->Source->getReg().asMCReg());
+      RegUnitsToInvalidate.insert(Dest.begin(), Dest.end());
+      RegUnitsToInvalidate.insert(Src.begin(), Src.end());
+    };
+
     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());
-        }
-        RegsToInvalidate.insert(I->second.DefRegs.begin(),
-                                I->second.DefRegs.end());
+        if (MachineInstr *MI = I->second.MI)
+          InvalidateCopy(MI);
+        if (MachineInstr *MI = I->second.LastSeenUseInCopy)
+          InvalidateCopy(MI);
       }
     }
-    for (MCRegister InvalidReg : RegsToInvalidate)
-      for (MCRegUnit Unit : TRI.regunits(InvalidReg))
-        Copies.erase(Unit);
+    for (MCRegUnit Unit : RegUnitsToInvalidate)
+      Copies.erase(Unit);
   }
 
   /// Clobber a single register, removing it from the tracker's copy maps.

llvm/test/CodeGen/AMDGPU/mcp-use-before-def.mir

@@ -0,0 +1,26 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
+# RUN: llc -mtriple=amdgcn-amd-amdhsa -verify-machineinstrs  -run-pass=machine-cp -o - %s | FileCheck %s
+
+# machine copy prop should not introduce use before def
+---
+name: back_copy_block
+body:           |
+  bb.0:
+    ; CHECK-LABEL: name: back_copy_block
+    ; CHECK: $vgpr20_vgpr21_vgpr22_vgpr23 = IMPLICIT_DEF
+    ; CHECK-NEXT: $vgpr10_vgpr11_vgpr12_vgpr13 = IMPLICIT_DEF
+    ; CHECK-NEXT: renamable $vgpr0_vgpr1 = V_MOV_B64_e64 killed $vgpr20_vgpr21, implicit $exec
+    ; CHECK-NEXT: renamable $vgpr20 = V_MOV_B32_e32 killed $vgpr1, implicit $exec
+    ; CHECK-NEXT: renamable $vgpr6_vgpr7_vgpr8_vgpr9 = COPY renamable $vgpr10_vgpr11_vgpr12_vgpr13
+    ; CHECK-NEXT: renamable $vgpr20 = V_MOV_B32_e32 killed $vgpr6, implicit $exec
+    ; CHECK-NEXT: S_ENDPGM 0, amdgpu_allvgprs
+    $vgpr20_vgpr21_vgpr22_vgpr23 = IMPLICIT_DEF
+    $vgpr10_vgpr11_vgpr12_vgpr13 = IMPLICIT_DEF
+    renamable $vgpr0_vgpr1 = V_MOV_B64_e64 killed renamable $vgpr20_vgpr21, implicit $exec
+    renamable $vgpr20 = V_MOV_B32_e32 killed renamable $vgpr1, implicit $exec
+    renamable $vgpr6_vgpr7_vgpr8_vgpr9 = COPY killed renamable $vgpr10_vgpr11_vgpr12_vgpr13
+    renamable $vgpr14_vgpr15 = COPY killed renamable $vgpr0_vgpr1
+    renamable $vgpr20 = V_MOV_B32_e32 killed renamable $vgpr6, implicit $exec
+    renamable $vgpr1_vgpr2_vgpr3_vgpr4 = COPY killed renamable $vgpr6_vgpr7_vgpr8_vgpr9
+    S_ENDPGM 0, amdgpu_allvgprs
+...