[AMDGPU] Enable copy from VCC to SHARED_BASE.

[carlobertolli]

No description provided.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: None (carlobertolli)

Changes

---

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

2 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/SIInstrInfo.cpp (+8)
• (modified) llvm/test/CodeGen/AMDGPU/sgpr-phys-copy.mir (+9)

diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 50447f48a628c..d1556cccf9524 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -845,6 +845,14 @@ void SIInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
     }
   }
 
+  if (AMDGPU::APERTURE_ClassRegClass.contains(DestReg)) {
+    if (SrcReg == AMDGPU::VCC) {
+      BuildMI(MBB, MI, DL, get(AMDGPU::S_MOV_B64), DestReg)
+        .addReg(SrcReg, getKillRegState(KillSrc));
+      return;
+    }
+  }
+
   if (RC == &AMDGPU::VGPR_32RegClass) {
     assert(AMDGPU::VGPR_32RegClass.contains(SrcReg) ||
            AMDGPU::SReg_32RegClass.contains(SrcReg) ||
diff --git a/llvm/test/CodeGen/AMDGPU/sgpr-phys-copy.mir b/llvm/test/CodeGen/AMDGPU/sgpr-phys-copy.mir
index f11fe4aa6e00e..78e8788c7f0f0 100644
--- a/llvm/test/CodeGen/AMDGPU/sgpr-phys-copy.mir
+++ b/llvm/test/CodeGen/AMDGPU/sgpr-phys-copy.mir
@@ -67,6 +67,15 @@ body:             |
     $vcc = COPY $src_shared_base
 ...
 
+---
+name: vcc_to_src_shared_base
+body:             |
+  bb.0:
+    ; GFX9-LABEL: name: vcc_to_src_shared_base
+    ; GFX9: $src_shared_base = S_MOV_B64 $vcc
+    $src_shared_base = COPY $vcc
+...
+
 ---
 name: sgpr96_aligned_src_dst
 body:             |

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[shiltian]

I don't think s_mov_b64 would allow SRC_SHARED_BASE as destination. It is not one of the categories in sdst.

[arsenm]

I don't think s_mov_b64 would allow SRC_SHARED_BASE as destination. It is not one of the categories in sdst.

Then the instruction definitions are wrong, because this should fail the verifier if it's not valid

[shiltian]

I think it does fail in the verifier, based on #163244 (comment).

[rampitec]

This is readonly register, like any src_. Yes, it shall not be used in SDST

[carlobertolli]

Section 5.2 of the MI300 programming guide indicates: "[..] 128-255 can only be used as sources."
shared_base is 235, so it cannot be used as destination.
This does not fail anywhere. I can run lit tests and they pass.
Is there a way to fix the "verifier"? @arsenm

[carlobertolli]

I ran it through the debugger looking at what happens in verifyInstruction in the same file. Both copies from VCC to SHARED_BASE and from SHARED_BASE to VCC get out at

4971 if (SIInstrInfo::isGenericOpcode(Opcode))
(gdb)
4972 return true;

There is no verification done on the src and dst registers if you get out of the function this early.

[carlobertolli]

The comment after that early exit seems relevant:

// FIXME: At this point the COPY verify is done only for non-ssa forms.
// Find a better property to recognize the point where instruction selection
// is just done.
// We can only enforce this check after SIFixSGPRCopies pass so that the
// illegal copies are legalized and thereafter we don't expect a pass
// inserting similar copies.

I'll leave this alone for the time being, but please @arsenm let me know if there is any extra verification you think we should be doing.

[jayfoad]

I ran it through the debugger looking at what happens in verifyInstruction in the same file. Both copies from VCC to SHARED_BASE and from SHARED_BASE to VCC get out at

4971 if (SIInstrInfo::isGenericOpcode(Opcode)) (gdb) 4972 return true;

There is no verification done on the src and dst registers if you get out of the function this early.

That sounds like you're looking at a COPY instruction? MachineVerfier should still fail for an S_MOV_B64 instruction like the one you generate in this patch.

[carlobertolli]

@jayfoad can you please point me to where the verification should happen?
I looked in MachineVerifier but the input is the COPY instruction, so no verification of the S_MOV_B64.

[jayfoad]

can you please point me to where the verification should happen? I looked in MachineVerifier but the input is the COPY instruction, so no verification of the S_MOV_B64.

You would have to run the verifier after you have converted the COPY to S_MOV_B64, so after the ExpandPostRAPseudos (aka postrapseudos) pass. You could run llc -verifymachineinstrs to verify after every pass.

[carlobertolli]

Thanks, below is what I see. It doesn't report that shared_base is non writable, but a problem with src_shared_base not being a SReg_64 register, which is as good I guess.
I will run this next time I want to write a codegen pass.

# After Post-RA pseudo instruction expansion pass
# Machine code for function vcc_to_src_shared_base: IsSSA, NoPHIs, NoVRegs

bb.0:
$src_shared_base = S_MOV_B64 $vcc

# End machine code for function vcc_to_src_shared_base.

*** Bad machine code: Operand has incorrect register class. ***

• function: vcc_to_src_shared_base
• basic block: %bb.0 (0x6331cd9270f8)
• instruction: $src_shared_base = S_MOV_B64 $vcc

*** Bad machine code: Illegal physical register for instruction ***

• function: vcc_to_src_shared_base
• basic block: %bb.0 (0x6331cd9270f8)
• instruction: $src_shared_base = S_MOV_B64 $vcc
• operand 0: $src_shared_base
  $src_shared_base is not a SReg_64 register.