MCP: Fixed bug with dest overlapping copy source

[tpr]

Summary

In MachineCopyPropagation, when propagating the source of a copy into
the operand of a later instruction, bail if a destination overlaps the
copy source. If the instruction where the substitution is happening is
also a copy, allowing the propagation confuses the tracking mechanism.

[arsenm]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

439–442

This will miss implicit defs. Should be checking MI.modifiesRegister maybe?

llvm/test/CodeGen/AMDGPU/mcp-overlap-after-propagation.mir

21

Should add another case where the relevant def is an implicit def

[tpr]

V2: Used modifiesRegister as suggested by Matt.

[tpr]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

439–442

Good idea. Makes the code simpler too.

llvm/test/CodeGen/AMDGPU/mcp-overlap-after-propagation.mir

21

I don't think I can test that. The bug only actually has an effect if the instruction that the copy is being folded into is itself a copy, and I can't think of a way then of there being an implicit def that overlaps the source of the first copy.

[arsenm]

LGTM

llvm/lib/CodeGen/MachineCopyPropagation.cpp

440

I'm pretty sure the << MI already includes the newline, so you can drop the extra one here

[tpr]

V3: Only bail if it is a copy and a partial def, to avoid spurious test changes.

[arsenm]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

438–439

Maybe should have a partiallyModifiesRegister?

llvm/lib/CodeGen/MachineCopyPropagation.cpp

if (MRI->isReserved(CopySrcReg) && !MRI->isConstantPhysReg(CopySrcReg))
       continue;
 
     if (!isForwardableRegClassCopy(*Copy, MI, OpIdx))
       continue;
 
     if (hasImplicitOverlap(MI, MOUse))
       continue;
 
+    // Check that the instruction is not a copy that partially overwrites the
+    // original copy source that we are about to use. The tracker mechanism
+    // cannot cope with that.
+    if (MI.isCopy() && MI.modifiesRegister(CopySrcReg, TRI) &&
+        !MI.definesRegister(CopySrcReg)) {
+      LLVM_DEBUG(dbgs() << "MCP: Copy source overlap with dest in " << MI);
+      continue;
+    }
+
     if (!DebugCounter::shouldExecute(FwdCounter)) {
       LLVM_DEBUG(dbgs() << "MCP: Skipping forwarding due to debug counter:\n  "
                         << MI);
       continue;
     }
 
     LLVM_DEBUG(dbgs() << "MCP: Replacing " << printReg(MOUse.getReg(), TRI)
                       << "\n     with " << printReg(CopySrcReg, TRI)

llvm/test/CodeGen/AMDGPU/mcp-overlap-after-propagation.mir

+# RUN: llc -march=amdgcn -mcpu=gfx1010 %s -o - -run-pass machine-cp -verify-machineinstrs | FileCheck %s
+#
+# The MachineCopyPropagation bug being tested propagates s[60:67] into the copy
+# into s[56:63], and then uses s[60:67] in the following
+# IMAGE_SAMPLE_V3_V2_gfx10, even though it has just overwritten half of it.
+
+# CHECK-LABEL: name: _amdgpu_ps_main
+# CHECK-NOT: IMAGE_SAMPLE_V3_V2_gfx10 {{.*}} $sgpr60_sgpr61_sgpr62_sgpr63_sgpr64_sgpr65_sgpr66_sgpr67
+# CHECK: IMAGE_SAMPLE_V3_V2_gfx10 {{.*}} $sgpr88_sgpr89_sgpr90_sgpr91_sgpr92_sgpr93_sgpr94_sgpr95
+
+---
+name:            _amdgpu_ps_main
+body:             |
+  bb.0:
+    successors:
+    liveins: $sgpr2, $sgpr3, $sgpr96, $sgpr97, $sgpr98, $sgpr99, $vgpr0, $vgpr1, $vgpr2, $vgpr3, $vgpr5, $vgpr70, $vgpr71
+
+    renamable $sgpr8_sgpr9 = S_GETPC_B64
+    renamable $sgpr8 = COPY killed renamable $sgpr2
+    renamable $sgpr60_sgpr61_sgpr62_sgpr63_sgpr64_sgpr65_sgpr66_sgpr67 = S_LOAD_DWORDX8_IMM renamable $sgpr8_sgpr9, 144, 0, 0 :: (invariant load 32, align 16, addrspace 4)
+    renamable $sgpr88_sgpr89_sgpr90_sgpr91_sgpr92_sgpr93_sgpr94_sgpr95 = COPY killed renamable $sgpr60_sgpr61_sgpr62_sgpr63_sgpr64_sgpr65_sgpr66_sgpr67
+    renamable $vgpr4 = IMAGE_GET_LOD_V1_V2_gfx10 renamable $vgpr70_vgpr71, renamable $sgpr88_sgpr89_sgpr90_sgpr91_sgpr92_sgpr93_sgpr94_sgpr95, renamable $sgpr96_sgpr97_sgpr98_sgpr99, 2, 1, 0, 0, 0, 0, 0, 0, 0, implicit $exec
+    renamable $sgpr56_sgpr57_sgpr58_sgpr59_sgpr60_sgpr61_sgpr62_sgpr63 = COPY killed renamable $sgpr88_sgpr89_sgpr90_sgpr91_sgpr92_sgpr93_sgpr94_sgpr95
+    renamable $vgpr12_vgpr13_vgpr14 = IMAGE_SAMPLE_V3_V2_gfx10 renamable $vgpr70_vgpr71, renamable $sgpr56_sgpr57_sgpr58_sgpr59_sgpr60_sgpr61_sgpr62_sgpr63, renamable $sgpr96_sgpr97_sgpr98_sgpr99, 7, 1, 0, 0, 0, 0, 0, 0, 0, 0, implicit $exec :: (dereferenceable load 12, align 16)
+    S_ENDPGM 0
+
+...