[AMDGPU] Restrict machine copy propagation from creating unaligned classes

[rampitec]

Summary

Fixes: SWDEV-326366

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4807

RI is already a class member

4808–4815

Why fix this up here when adjustAllocatableRegClass can return the aligned version in the first place?

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4748

This reservedRegsFrozen check is probably obsolete

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4774

Why can't you just select the aligned class above to begin with?

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4774

This is only VGPRs, and not all VGPRs. Then it will use a condition at every case, while the code below is still needed.

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4774

I don't think RC can be null here

4774

I don't understand. What do you mean not all VGPRs? You need the aligned class or not?

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4774

RC can be null here and it does for many tests.

The switch above only covers AV and only certain sizes. It does not cover neither vgprs nor agprs. The very first test I have added would not pass because it uses VReg_64.

[rampitec]

There seems to be some confusion here, it is not because AV classes has become allocatable. It is an issue from day 1 aligned classes were introduced.

[cdevadas]

In general, invoking adjustAllocatableRegClass post-regalloc doesn't sound good to me.
I'm in favor of the code that you added to get the aligned regclasses.
But what's the need to adjust the AV operands beforehand? They'll meet the condition as we have the _align2 versions for the superclasses available now.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4777

Factor this out into a separate function?
We have the following code already used at least 2 more places in our backend.

llvm/test/CodeGen/AMDGPU/mcp-aligned-vgprs.mir

13

Why does such a copy exist in the first place?
The register pair given for 64-bit Dst operand of COPY itself is unaligned.
One situation I can imagine is when the Dst is the odd subpart of a higher tuple.
But in that case, shouldn't that info be given in the instruction as an implicit operand?

[rampitec]

llvm/test/CodeGen/AMDGPU/mcp-aligned-vgprs.mir

13

This is RA created copy of vreg_96:sub1_sub2. A perfectly valid and aligned case.

[rampitec]

In D121491#3376885, @cdevadas wrote:

In general, invoking adjustAllocatableRegClass post-regalloc doesn't sound good to me.

Which we do already.

[rampitec]

Factored out getProperlyAlignedRC().

[rampitec]

In D121491#3376885, @cdevadas wrote:

But what's the need to adjust the AV operands beforehand? They'll meet the condition as we have the _align2 versions for the superclasses available now.

MCP sees a physreg which can be propagated, like in the test: aligned physreg copied into an unaligned physreg. It queries TII of get instruction's operand regclass and then checks if copy destination fits that regclass. If it does MCP performs propagation. Therefore, to prevent it we shall adjust the regclass extracted from the operand desc.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4777

Extracted, although I do not see other places where we need exactly the same interface.

[cdevadas]

LGTM except for the typo.

llvm/lib/Target/AMDGPU/SIRegisterInfo.h

382

s/correcsponding/corresponding

[rampitec]

Fixed typo.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

unsigned SIInstrInfo::getVALUOp(const MachineInstr &MI) const {
   case AMDGPU::S_FLBIT_I32: return AMDGPU::V_FFBH_I32_e64;
   case AMDGPU::S_CBRANCH_SCC0: return AMDGPU::S_CBRANCH_VCCZ;
   case AMDGPU::S_CBRANCH_SCC1: return AMDGPU::S_CBRANCH_VCCNZ;
   }
   llvm_unreachable(
       "Unexpected scalar opcode without corresponding vector one!");
 }
 
-static unsigned adjustAllocatableRegClass(const GCNSubtarget &ST,
-                                          const MachineRegisterInfo &MRI,
-                                          const MCInstrDesc &TID,
-                                          unsigned RCID,
-                                          bool IsAllocatable) {
+static const TargetRegisterClass *
+adjustAllocatableRegClass(const GCNSubtarget &ST, const SIRegisterInfo &RI,
+                          const MCInstrDesc &TID, unsigned RCID,
   if ((IsAllocatable || !ST.hasGFX90AInsts() || !MRI.reservedRegsFrozen()) &&
       (((TID.mayLoad() || TID.mayStore()) &&
         !(TID.TSFlags & SIInstrFlags::VGPRSpill)) ||
        (TID.TSFlags & (SIInstrFlags::DS | SIInstrFlags::MIMG)))) {
     switch (RCID) {
-    case AMDGPU::AV_32RegClassID: return AMDGPU::VGPR_32RegClassID;
-    case AMDGPU::AV_64RegClassID: return AMDGPU::VReg_64RegClassID;
-    case AMDGPU::AV_96RegClassID: return AMDGPU::VReg_96RegClassID;
-    case AMDGPU::AV_128RegClassID: return AMDGPU::VReg_128RegClassID;
-    case AMDGPU::AV_160RegClassID: return AMDGPU::VReg_160RegClassID;
+    case AMDGPU::AV_32RegClassID:
+      RCID = AMDGPU::VGPR_32RegClassID;
+      break;
+    case AMDGPU::AV_64RegClassID:
+      RCID = AMDGPU::VReg_64RegClassID;
+      break;
+    case AMDGPU::AV_96RegClassID:
+      RCID = AMDGPU::VReg_96RegClassID;
+      break;
+    case AMDGPU::AV_128RegClassID:
+      RCID = AMDGPU::VReg_128RegClassID;
+      break;
+    case AMDGPU::AV_160RegClassID:
+      RCID = AMDGPU::VReg_160RegClassID;
+      break;
     default:
       break;
     }
   }
-  return RCID;
+
+  return RI.getProperlyAlignedRC(RI.getRegClass(RCID));
 }
 
 const TargetRegisterClass *SIInstrInfo::getRegClass(const MCInstrDesc &TID,
     unsigned OpNum, const TargetRegisterInfo *TRI,
     const MachineFunction &MF)
   const {
   if (OpNum >= TID.getNumOperands())
     return nullptr;
   auto RegClass = TID.OpInfo[OpNum].RegClass;
   bool IsAllocatable = false;
   if (TID.TSFlags & (SIInstrFlags::DS | SIInstrFlags::FLAT)) {
     // vdst and vdata should be both VGPR or AGPR, same for the DS instructions
const int DataIdx = AMDGPU::getNamedOperandIdx(TID.Opcode,
         (TID.TSFlags & SIInstrFlags::DS) ? AMDGPU::OpName::data0
                                          : AMDGPU::OpName::vdata);
     if (DataIdx != -1) {
       IsAllocatable = VDstIdx != -1 ||
                       AMDGPU::getNamedOperandIdx(TID.Opcode,
                                                  AMDGPU::OpName::data1) != -1;
     }
   }
-  RegClass = adjustAllocatableRegClass(ST, MF.getRegInfo(), TID, RegClass,
-                                       IsAllocatable);
-  return RI.getRegClass(RegClass);
+  return adjustAllocatableRegClass(ST, RI, MF.getRegInfo(), TID, RegClass,
 }
 
 const TargetRegisterClass *SIInstrInfo::getOpRegClass(const MachineInstr &MI,
                                                       unsigned OpNo) const {
   const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
   const MCInstrDesc &Desc = get(MI.getOpcode());
   if (MI.isVariadic() || OpNo >= Desc.getNumOperands() ||
       Desc.OpInfo[OpNo].RegClass == -1) {
     Register Reg = MI.getOperand(OpNo).getReg();
 
     if (Reg.isVirtual())
       return MRI.getRegClass(Reg);
     return RI.getPhysRegClass(Reg);
   }
 
   unsigned RCID = Desc.OpInfo[OpNo].RegClass;
-  RCID = adjustAllocatableRegClass(ST, MRI, Desc, RCID, true);
-  return RI.getRegClass(RCID);
+  return adjustAllocatableRegClass(ST, RI, MRI, Desc, RCID, true);
 }
 
 void SIInstrInfo::legalizeOpWithMove(MachineInstr &MI, unsigned OpIdx) const {
   MachineBasicBlock::iterator I = MI;
   MachineBasicBlock *MBB = MI.getParent();
   MachineOperand &MO = MI.getOperand(OpIdx);
   MachineRegisterInfo &MRI = MBB->getParent()->getRegInfo();
   unsigned RCID = get(MI.getOpcode()).OpInfo[OpIdx].RegClass;

llvm/lib/Target/AMDGPU/SIRegisterInfo.h

public:
   // For a given 16 bit \p Reg \returns a 32 bit register holding it.
   // \returns \p Reg otherwise.
   MCPhysReg get32BitRegister(MCPhysReg Reg) const;
 
   // Returns true if a given register class is properly aligned for
   // the subtarget.
   bool isProperlyAlignedRC(const TargetRegisterClass &RC) const;
 
+  // Given \p RC returns corresponding aligned register class if required
+  // by the subtarget.
+  const TargetRegisterClass *
+  getProperlyAlignedRC(const TargetRegisterClass *RC) const;
+
   /// Return all SGPR128 which satisfy the waves per execution unit requirement
   /// of the subtarget.
   ArrayRef<MCPhysReg> getAllSGPR128(const MachineFunction &MF) const;
 
   /// Return all SGPR64 which satisfy the waves per execution unit requirement
   /// of the subtarget.
   ArrayRef<MCPhysReg> getAllSGPR64(const MachineFunction &MF) const;
 

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

if (isAGPRClass(&RC))
     return RC.hasSuperClassEq(getAGPRClassForBitWidth(getRegSizeInBits(RC)));
   if (isVectorSuperClass(&RC))
     return RC.hasSuperClassEq(
         getVectorSuperClassForBitWidth(getRegSizeInBits(RC)));
 
   return true;
 }
 
+const TargetRegisterClass *
+SIRegisterInfo::getProperlyAlignedRC(const TargetRegisterClass *RC) const {
+  if (!RC || !ST.needsAlignedVGPRs())
+    return RC;
+
+  unsigned Size = getRegSizeInBits(*RC);
+  if (Size <= 32)
+    return RC;
+
+  if (isVGPRClass(RC))
+    return getAlignedVGPRClassForBitWidth(Size);
+  if (isAGPRClass(RC))
+    return getAlignedAGPRClassForBitWidth(Size);
+  if (isVectorSuperClass(RC))
+    return getAlignedVectorSuperClassForBitWidth(Size);
+
+  return RC;
+}
+
 bool SIRegisterInfo::isConstantPhysReg(MCRegister PhysReg) const {
   switch (PhysReg) {
   case AMDGPU::SGPR_NULL:
   case AMDGPU::SRC_SHARED_BASE:
   case AMDGPU::SRC_PRIVATE_BASE:
   case AMDGPU::SRC_SHARED_LIMIT:
   case AMDGPU::SRC_PRIVATE_LIMIT:
     return true;

llvm/test/CodeGen/AMDGPU/mcp-aligned-vgprs.mir

+# RUN: llc -march=amdgcn -mcpu=gfx90a -run-pass=machine-cp -verify-machineinstrs -o - %s | FileCheck --check-prefix=GCN %s
+
+# GCN-LABEL: name: mcp_aligned_vgprs
+# GCN: $vgpr0_vgpr1 = V_PK_MUL_F32 0, $sgpr0_sgpr1
+# GCN: $vgpr3_vgpr4 = COPY killed renamable $vgpr0_vgpr1
+---
+name:            mcp_aligned_vgprs
+body:             |
+  bb.0.entry:
+
+    renamable $vgpr0_vgpr1 = V_PK_MUL_F32 0, $sgpr0_sgpr1, 0, 0, 0, 0, 0, 0, 0, implicit $mode, implicit $exec
+    renamable $vgpr3_vgpr4 = COPY killed renamable $vgpr0_vgpr1
+    S_ENDPGM 0, implicit $vgpr3_vgpr4
+...
+
+# GCN-LABEL: name: mcp_aligned_agprs
+# GCN: $agpr0_agpr1 = GLOBAL_LOAD_DWORDX2_SADDR $sgpr0_sgpr1, $vgpr10
+# GCN: $agpr3_agpr4 = COPY killed renamable $agpr0_agpr1
+---
+name:            mcp_aligned_agprs
+body:             |
+  bb.0.entry:
+
+    renamable $agpr0_agpr1 = GLOBAL_LOAD_DWORDX2_SADDR $sgpr0_sgpr1, $vgpr10, 0, 0, implicit $exec
+    renamable $agpr3_agpr4 = COPY killed renamable $agpr0_agpr1
+    S_ENDPGM 0, implicit $agpr3_agpr4
+
+...