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

[Jeffrey Byrnes via Phabricator via llvm-commits]

jrbyrnes created this revision.
jrbyrnes added reviewers: bzEq, arsenm.
Herald added subscribers: kerbowa, hiraditya, jvesely.
Herald added a project: All.
jrbyrnes requested review of this revision.
Herald added subscribers: llvm-commits, wdng.
Herald added a project: LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157564

Files:
  llvm/lib/CodeGen/MachineCopyPropagation.cpp
  llvm/test/CodeGen/AMDGPU/mcp-use-before-def.mir


Index: llvm/test/CodeGen/AMDGPU/mcp-use-before-def.mir
===================================================================
--- /dev/null
+++ llvm/test/CodeGen/AMDGPU/mcp-use-before-def.mir
@@ -0,0 +1,28 @@
+# 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
+...
+## NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+# CHECK: {{.*}}
Index: llvm/lib/CodeGen/MachineCopyPropagation.cpp
===================================================================
--- llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -109,6 +109,7 @@
   struct CopyInfo {
     MachineInstr *MI, *LastSeenUseInCopy;
     SmallVector<MCRegister, 4> DefRegs;
+    SmallVector<MCRegister, 4> SrcRegs;
     bool Avail;
   };
 
@@ -151,6 +152,8 @@
         }
         RegsToInvalidate.insert(I->second.DefRegs.begin(),
                                 I->second.DefRegs.end());
+        RegsToInvalidate.insert(I->second.SrcRegs.begin(),
+                                I->second.SrcRegs.end());
       }
     }
     for (MCRegister InvalidReg : RegsToInvalidate)
@@ -167,6 +170,7 @@
         // When we clobber the source of a copy, we need to clobber everything
         // it defined.
         markRegsUnavailable(I->second.DefRegs, TRI);
+        markRegsUnavailable(I->second.SrcRegs, TRI);
         // When we clobber the destination of a copy, we need to clobber the
         // whole register it defined.
         if (MachineInstr *MI = I->second.MI) {
@@ -193,15 +197,17 @@
 
     // Remember Def is defined by the copy.
     for (MCRegUnit Unit : TRI.regunits(Def))
-      Copies[Unit] = {MI, nullptr, {}, true};
+      Copies[Unit] = {MI, nullptr, {}, {}, true};
 
     // Remember source that's copied to Def. Once it's clobbered, then
     // it's no longer available for copy propagation.
     for (MCRegUnit Unit : TRI.regunits(Src)) {
-      auto I = Copies.insert({Unit, {nullptr, nullptr, {}, false}});
+      auto I = Copies.insert({Unit, {nullptr, nullptr, {}, {}, false}});
       auto &Copy = I.first->second;
       if (!is_contained(Copy.DefRegs, Def))
         Copy.DefRegs.push_back(Def);
+      if (!is_contained(Copy.SrcRegs, Src))
+        Copy.SrcRegs.push_back(Src);
       Copy.LastSeenUseInCopy = MI;
     }
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D157564.548819.patch
Type: text/x-patch
Size: 3778 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230809/3084ad42/attachment.bin>