[AMDGPU] All GWS instructions need aligned VGPR on gfx90a

[rampitec]

Summary

Fixes: SWDEV-288006

[arsenm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

4228–4237

Should just constrain the register class to an even aligned 32-bit register class

[rampitec]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

4228–4237

That was the first thing I did. It is much more code to handle just these 3 instructions, and it does not really work, I cannot prevent SIFoldOperands from replacing a register with unaligned one. Then isLegalRegOperand() should also handle this situation, which in turn would mean duplicating ds_gws opcodes with a different operand RC, handling new operand type in asm/disasm and other places.

On top of that such RC must have OtherVT as the only supported VT so that ISel will not use it after a call to getMinimalPhysRegClass(). This RC also will make it a quest to implement something line getCompatibleSubRegClass() and similar when an odd subreg is requested.

This is really an overkill.

[rampitec]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

4228–4237

You can take a look at D103213 which does that. I was able to get away w/o new operand types, but I still believe that is an overkill to do it that way.

[rampitec]

JBTW, this patch directly reflects what's actually happen in HW. Even though these 3 instructions read 32 bit as a source internally they request to read 64 bit which in turn triggered alignment requirement. I assume this is really modeled as "read dword as an operand and have an implicit read of a superreg".

[rampitec]

Added verifier check.

[arsenm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

4228–4237

SIFoldOperands should be respecting operand constraints

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4351–4353

The SSA check doesn't make sense, the class is unrelated

[rampitec]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

4228–4237

And for that I had to write a pipe of code in the D103213 to handle it in the isOperandLegal(), which as I said is an overkill.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4351–4353

Any idea how to skip verifier after selection with gisel and before finalize lowering? We should have lot more problems like this as finalizeLowering() calls legalizeOperands() for many more instructions, we just do not have enough tests I guess.

[rampitec]

Fix tidy warning.

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4351–4353

Here is what happens with gisel:

# *** IR Dump After InstructionSelect (instruction-select) ***:
# Machine code for function gws_init_odd_agpr: IsSSA, TracksLiveness, Legalized, RegBankSelected, Selected
Function Live Ins: $sgpr0_sgpr1 in %1, $sgpr2 in %6
...
  DS_GWS_INIT %16:vgpr_32, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
...
# *** IR Dump After Finalize ISel and expand pseudo-instructions (finalize-isel) ***:
# Machine code for function gws_init_odd_agpr: IsSSA, TracksLiveness, Legalized, RegBankSelected, Selected
Function Live Ins: $sgpr0_sgpr1 in %1, $sgpr2 in %6
...
  BUNDLE implicit %19:vreg_64_align2, implicit $m0, implicit $exec {
    DS_GWS_INIT %19.sub0:vreg_64_align2, 0, implicit $m0, implicit $exec, implicit %19:vreg_64_align2 :: (store 4 into custom "GWSResource")
    S_WAITCNT 0
  }

If verifier kiks-in before finalizeLowering the instruction is not yet fixed, just like any other instruction which is supposed to be legalized there.

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

4351–4353

Oh right, this problem. We currently have an ugly hack where we disable the verifier for selectiondag before finalize lowering, but I would like to avoid spreading that problem.

Can you adjust the class directly in selectDSGWSIntrinsic for gisel?

[rampitec]

Same code added to selectDSGWSIntrinsic().

[arsenm]

LGTM plus comment

llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp

1394

Should add a comment explaining this hack

[rampitec]

Added comment.

llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp

bool AMDGPUInstructionSelector::selectDSGWSIntrinsic(MachineInstr &MI,
 
   // The resource id offset is computed as (<isa opaque base> + M0[21:16] +
   // offset field) % 64. Some versions of the programming guide omit the m0
   // part, or claim it's from offset 0.
   auto MIB = BuildMI(*MBB, &MI, DL, TII.get(gwsIntrinToOpcode(IID)));
 
   if (HasVSrc) {
     Register VSrc = MI.getOperand(1).getReg();
+
+    if (STI.needsAlignedVGPRs()) {
+      // Add implicit aligned super-reg to force alignment on the data operand.
+      Register Undef = MRI->createVirtualRegister(&AMDGPU::VGPR_32RegClass);
+      BuildMI(*MBB, &*MIB, DL, TII.get(AMDGPU::IMPLICIT_DEF), Undef);
+      Register NewVR =
+          MRI->createVirtualRegister(&AMDGPU::VReg_64_Align2RegClass);
+      BuildMI(*MBB, &*MIB, DL, TII.get(AMDGPU::REG_SEQUENCE), NewVR)
+          .addReg(VSrc, 0, MI.getOperand(1).getSubReg())
+          .addImm(AMDGPU::sub0)
+          .addReg(Undef)
+          .addImm(AMDGPU::sub1);
+      MIB.addReg(NewVR, 0, AMDGPU::sub0);
+      MIB.addReg(NewVR, RegState::Implicit);
+    } else {
       MIB.addReg(VSrc);
+    }
+
     if (!RBI.constrainGenericRegister(VSrc, AMDGPU::VGPR_32RegClass, *MRI))
       return false;
   }
 
   MIB.addImm(ImmOffset)
      .cloneMemRefs(MI);
 
   MI.eraseFromParent();

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

if (NeedClampOperand)
       I.addImm(0); // clamp bit for e64 encoding
 
     TII->legalizeOperands(*I);
 
     MI.eraseFromParent();
     return BB;
   }
   case AMDGPU::DS_GWS_INIT:
-  case AMDGPU::DS_GWS_SEMA_V:
   case AMDGPU::DS_GWS_SEMA_BR:
+  case AMDGPU::DS_GWS_BARRIER:
+    if (Subtarget->needsAlignedVGPRs()) {
+      // Add implicit aligned super-reg to force alignment on the data operand.
+      const DebugLoc &DL = MI.getDebugLoc();
+      MachineRegisterInfo &MRI = BB->getParent()->getRegInfo();
+      const SIRegisterInfo *TRI = Subtarget->getRegisterInfo();
+      MachineOperand *Op = TII->getNamedOperand(MI, AMDGPU::OpName::data0);
+      Register DataReg = Op->getReg();
+      bool IsAGPR = TRI->isAGPR(MRI, DataReg);
+      Register Undef = MRI.createVirtualRegister(
+          IsAGPR ? &AMDGPU::AGPR_32RegClass : &AMDGPU::VGPR_32RegClass);
+      BuildMI(*BB, MI, DL, TII->get(AMDGPU::IMPLICIT_DEF), Undef);
+      Register NewVR =
+          MRI.createVirtualRegister(IsAGPR ? &AMDGPU::AReg_64_Align2RegClass
+                                           : &AMDGPU::VReg_64_Align2RegClass);
+      BuildMI(*BB, MI, DL, TII->get(AMDGPU::REG_SEQUENCE), NewVR)
+          .addReg(DataReg, 0, Op->getSubReg())
+          .addImm(AMDGPU::sub0)
+          .addReg(Undef)
+          .addImm(AMDGPU::sub1);
+      Op->setReg(NewVR);
+      Op->setSubReg(AMDGPU::sub0);
+      MI.addOperand(MachineOperand::CreateReg(NewVR, false, true));
+    }
+    LLVM_FALLTHROUGH;
+  case AMDGPU::DS_GWS_SEMA_V:
   case AMDGPU::DS_GWS_SEMA_P:
   case AMDGPU::DS_GWS_SEMA_RELEASE_ALL:
-  case AMDGPU::DS_GWS_BARRIER:
     // A s_waitcnt 0 is required to be the instruction immediately following.
     if (getSubtarget()->hasGWSAutoReplay()) {
       bundleInstWithWaitcnt(MI);
       return BB;
     }
 
     return emitGWSMemViolTestLoop(MI, BB);
   case AMDGPU::S_SETREG_B32: {

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

if (ST.hasGFX90AInsts()) {
           (Data2 && RI.isAGPR(MRI, Data2->getReg()))) {
         ErrInfo = "Invalid register class: "
                   "agpr loads and stores not supported on this GPU";
         return false;
       }
     }
   }
 
+  if (ST.needsAlignedVGPRs() &&
+      (MI.getOpcode() == AMDGPU::DS_GWS_INIT ||
+       MI.getOpcode() == AMDGPU::DS_GWS_SEMA_BR ||
+       MI.getOpcode() == AMDGPU::DS_GWS_BARRIER)) {
+    const MachineOperand *Op = getNamedOperand(MI, AMDGPU::OpName::data0);
+    Register Reg = Op->getReg();
+    bool Aligned = true;
+    if (Reg.isPhysical()) {
+      Aligned = !(RI.getHWRegIndex(Reg) & 1);
+    } else {
+      const TargetRegisterClass &RC = *MRI.getRegClass(Reg);
+      Aligned = RI.getRegSizeInBits(RC) > 32 && RI.isProperlyAlignedRC(RC) &&
+                !(RI.getChannelFromSubReg(Op->getSubReg()) & 1);
+    }
+
+    if (!Aligned) {
+      ErrInfo = "Subtarget requires even aligned vector registers "
+                "for DS_GWS instructions";
+      return false;
+    }
+  }
+
   return true;
 }
 
 unsigned SIInstrInfo::getVALUOp(const MachineInstr &MI) const {
   switch (MI.getOpcode()) {
   default: return AMDGPU::INSTRUCTION_LIST_END;
   case AMDGPU::REG_SEQUENCE: return AMDGPU::REG_SEQUENCE;
   case AMDGPU::COPY: return AMDGPU::COPY;

llvm/test/CodeGen/AMDGPU/ds_gws_align.ll

+; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx908 -o - -verify-machineinstrs < %s | FileCheck --check-prefixes=GCN,GFX908 %s
+; RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx90a -o - -verify-machineinstrs < %s | FileCheck --check-prefixes=GCN,GFX90A %s
+; RUN: llc -global-isel -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx908 -o - -verify-machineinstrs < %s | FileCheck --check-prefixes=GCN,GFX908 %s
+; RUN: llc -global-isel -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx90a -o - -verify-machineinstrs < %s | FileCheck --check-prefixes=GCN,GFX90A %s
+
+; GCN-LABEL: {{^}}gws_init_odd_reg:
+; GFX908-DAG: ds_gws_init v1 gds
+; GFX90A-DAG: ds_gws_init v2 gds
+; GCN-DAG:    ds_gws_init v0 gds
+define amdgpu_ps void @gws_init_odd_reg(<2 x i32> %arg) {
+  %vgpr.0 = extractelement <2 x i32> %arg, i32 0
+  %vgpr.1 = extractelement <2 x i32> %arg, i32 1
+  call void @llvm.amdgcn.ds.gws.init(i32 %vgpr.0, i32 0)
+  call void @llvm.amdgcn.ds.gws.init(i32 %vgpr.1, i32 0)
+  ret void
+}
+
+; GCN-LABEL: {{^}}gws_sema_br_odd_reg:
+; GFX908-DAG: ds_gws_sema_br v1 gds
+; GFX90A-DAG: ds_gws_sema_br v2 gds
+; GCN-DAG:    ds_gws_sema_br v0 gds
+define amdgpu_ps void @gws_sema_br_odd_reg(<2 x i32> %arg) {
+  %vgpr.0 = extractelement <2 x i32> %arg, i32 0
+  %vgpr.1 = extractelement <2 x i32> %arg, i32 1
+  call void @llvm.amdgcn.ds.gws.sema.br(i32 %vgpr.0, i32 0)
+  call void @llvm.amdgcn.ds.gws.sema.br(i32 %vgpr.1, i32 0)
+  ret void
+}
+
+; GCN-LABEL: {{^}}gws_barrier_odd_reg:
+; GFX908-DAG: ds_gws_barrier v1 gds
+; GFX90A-DAG: ds_gws_barrier v2 gds
+; GCN-DAG:    ds_gws_barrier v0 gds
+define amdgpu_ps void @gws_barrier_odd_reg(<2 x i32> %arg) {
+  %vgpr.0 = extractelement <2 x i32> %arg, i32 0
+  %vgpr.1 = extractelement <2 x i32> %arg, i32 1
+  call void @llvm.amdgcn.ds.gws.barrier(i32 %vgpr.0, i32 0)
+  call void @llvm.amdgcn.ds.gws.barrier(i32 %vgpr.1, i32 0)
+  ret void
+}
+
+; GCN-LABEL: {{^}}gws_init_odd_agpr:
+; GFX908-COUNT-2: ds_gws_init v{{[0-9]+}} gds
+; GFX90A-COUNT-2: ds_gws_init {{[va][0-9]?[02468]}} gds
+define amdgpu_ps void @gws_init_odd_agpr(<4 x i32> %arg) {
+bb:
+  %mai = tail call <4 x i32> @llvm.amdgcn.mfma.i32.4x4x4i8(i32 1, i32 2, <4 x i32> %arg, i32 0, i32 0, i32 0)
+  %agpr.0 = extractelement <4 x i32> %mai, i32 0
+  %agpr.1 = extractelement <4 x i32> %mai, i32 1
+  call void @llvm.amdgcn.ds.gws.init(i32 %agpr.0, i32 0)
+  call void @llvm.amdgcn.ds.gws.init(i32 %agpr.1, i32 0)
+  ret void
+}
+
+declare void @llvm.amdgcn.ds.gws.init(i32, i32)
+declare void @llvm.amdgcn.ds.gws.sema.br(i32, i32)
+declare void @llvm.amdgcn.ds.gws.barrier(i32, i32)
+declare <4 x i32> @llvm.amdgcn.mfma.i32.4x4x4i8(i32, i32, <4 x i32>, i32, i32, i32)

llvm/test/CodeGen/AMDGPU/verify-ds-gws-align.mir

+# RUN: not --crash llc -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx90a -run-pass=machineverifier -o /dev/null %s 2>&1 | FileCheck -check-prefix=GFX90A-ERR %s
+
+# GFX90A-ERR: *** Bad machine code: Subtarget requires even aligned vector registers for DS_GWS instructions ***
+# GFX90A-ERR: DS_GWS_INIT killed %0.sub1:areg_128_align2, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+# GFX90A-ERR: *** Bad machine code: Subtarget requires even aligned vector registers for DS_GWS instructions ***
+# GFX90A-ERR: DS_GWS_INIT killed %0.sub3:areg_128_align2, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+# GFX90A-ERR: *** Bad machine code: Subtarget requires even aligned vector registers for DS_GWS instructions ***
+# GFX90A-ERR: DS_GWS_SEMA_BR killed %1.sub1:vreg_64_align2, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+# GFX90A-ERR: *** Bad machine code: Subtarget requires even aligned vector registers for DS_GWS instructions ***
+# GFX90A-ERR: DS_GWS_BARRIER killed %2.sub0:vreg_64, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+# GFX90A-ERR: *** Bad machine code: Subtarget requires even aligned vector registers for DS_GWS instructions ***
+# GFX90A-ERR: DS_GWS_INIT killed %3:vgpr_32, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+# GFX90A-ERR: *** Bad machine code: Subtarget requires even aligned vector registers for DS_GWS instructions ***
+# GFX90A-ERR: DS_GWS_INIT $vgpr1, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+# GFX90A-ERR: *** Bad machine code: Subtarget requires even aligned vector registers for DS_GWS instructions ***
+# GFX90A-ERR: DS_GWS_INIT $agpr1, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+---
+name:            gws_odd_vgpr
+body:             |
+  bb.0:
+    %0:areg_128_align2 = IMPLICIT_DEF
+    DS_GWS_INIT killed %0.sub1, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+    %0:areg_128_align2 = IMPLICIT_DEF
+    DS_GWS_INIT killed %0.sub3, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+    %1:vreg_64_align2 = IMPLICIT_DEF
+    DS_GWS_SEMA_BR killed %1.sub1, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+    %2:vreg_64 = IMPLICIT_DEF
+    DS_GWS_BARRIER killed %2.sub0, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+    %3:vgpr_32 = IMPLICIT_DEF
+    DS_GWS_INIT killed %3, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+    $vgpr1 = IMPLICIT_DEF
+    DS_GWS_INIT $vgpr1, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+    $agpr1 = IMPLICIT_DEF
+    DS_GWS_INIT $agpr1, 0, implicit $m0, implicit $exec :: (store 4 into custom "GWSResource")
+    S_ENDPGM 0
+
+...