[AMDGPU] Fix subreg check in the SIFixSGPRCopies

[rampitec]

It checks for the copy of subregs, but it checks destination which may never happen in SSA. It misses the subreg check and happily produces S_MOV_B64 out of a subreg COPY.

The affected test should have never been formed in the first place because the pass is running in SSA and copies into a subreg shall never happen.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Stanislav Mekhanoshin (rampitec)

Changes

It checks for the copy of subregs, but it checks destination which may never happen in SSA. It misses the subreg check and happily produces S_MOV_B64 out of a subreg COPY.

The affected test should have never been formed in the first place because the pass is running in SSA and copies into a subreg shall never happen.

---

Full diff: https://github.com/llvm/llvm-project/pull/70007.diff

3 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp (+1-1)
• (modified) llvm/test/CodeGen/AMDGPU/fix-sgpr-copies.mir (+11)
• (modified) llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir (+1-2)

diff --git a/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp b/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
index 60cd9d4c3c35a27..b32ed9fef5dd34e 100644
--- a/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
+++ b/llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
@@ -357,7 +357,7 @@ static bool isSafeToFoldImmIntoCopy(const MachineInstr *Copy,
     return false;
 
   // FIXME: Handle copies with sub-regs.
-  if (Copy->getOperand(0).getSubReg())
+  if (Copy->getOperand(1).getSubReg())
     return false;
 
   switch (MoveImm->getOpcode()) {
diff --git a/llvm/test/CodeGen/AMDGPU/fix-sgpr-copies.mir b/llvm/test/CodeGen/AMDGPU/fix-sgpr-copies.mir
index 11cabedd27b7bd4..8d0a9899b5dbcc7 100644
--- a/llvm/test/CodeGen/AMDGPU/fix-sgpr-copies.mir
+++ b/llvm/test/CodeGen/AMDGPU/fix-sgpr-copies.mir
@@ -222,3 +222,14 @@ body:             |
     %4:sreg_32 = COPY %3:vgpr_32
     %5:sreg_32 = nofpexcept S_FMAC_F32 killed %4:sreg_32, %1:sreg_32, %2:sreg_32, implicit $mode
 ...
+
+---
+# GCN-LABEL: name: moveimm_subreg_input
+# GCN: %0:vreg_64 = V_MOV_B64_PSEUDO 0, implicit $exec
+# GCN: :vgpr_32 = COPY %0.sub0
+name:            moveimm_subreg_input
+body:             |
+  bb.0:
+    %0:vreg_64 = V_MOV_B64_PSEUDO 0, implicit $exec
+    %1:sreg_32 = COPY %0.sub0
+...
diff --git a/llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir b/llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir
index 59d5d7883a03268..6f42f4c53a03cf2 100644
--- a/llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir
+++ b/llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir
@@ -34,8 +34,7 @@ body:             |
 
 ---
 # GCN-LABEL: name: phi_moveimm_subreg_input
-# GCN-NOT: %{{[0-9]+}}:sreg_64 = PHI %{{[0-9]+}}, %bb.3, %{{[0-9]+}}, %bb.1
-# GCN: %{{[0-9]+}}:vreg_64 = PHI %{{[0-9]+}}, %bb.3, %{{[0-9]+}}, %bb.1
+# GCN: %{{[0-9]+}}:sreg_64 = PHI %{{[0-9]+}}, %bb.3, %{{[0-9]+}}, %bb.1
 name:            phi_moveimm_subreg_input
 tracksRegLiveness: true
 body:             |

[jayfoad]

LGTM.

It checks for the copy of subregs, but it checks destination which may never happen in SSA.

Maybe MachineVerifier should check that.

[rampitec]

LGTM.

It checks for the copy of subregs, but it checks destination which may never happen in SSA.

Maybe MachineVerifier should check that.

Maybe as a global check, that a def cannot have subreg in SSA?

[rampitec]

LGTM.

It checks for the copy of subregs, but it checks destination which may never happen in SSA.

Maybe MachineVerifier should check that.

Maybe as a global check, that a def cannot have subreg in SSA?

Actually I've seen these checks. But it will not help here with the MIR test, it just does not say if we are in SSA.