[AMDGPU] Fix a weird WWM intrinsic issue.

[sheredom]

Summary

I found a really strange WWM issue through a very convoluted shader that essentially boils down to a bug in SIInstrInfo where canReadVGPR did not correctly identify that WWM is like a copy and can have a VGPR as its source.

[nhaehnle]

Interesting.

The only user of canReadVGPR is addUsersToMoveToVALUWorklist. Since the intended semantics of canReadVGPR aren't at all clear from the name, might I suggesting folding it into its only user?

Following the chain further down, getOpRegClass also doesn't have particularly clear semantics. It's at least documented in the method's comment, but still: a function which returns an instruction's opcode's intrinsic regclass, but then falls back to the regclass that just happens to be there seems a bit misguided to me.

I think getOpRegClass should at most be a convenience function that returns the opcode's regclass if it exists, or nullptr otherwise. From a quick glance, it looks like most (all?) users should be fine with either using an opcode's intrinsic regclass (e.g. in getOpSize) or, more commonly, with just taking the MachineOperand's register class.

In the case of addUsersToMoveToVALUWorklist, the code should just check the users' opcode's intrinsic regclass; if it exists, add the user based on whether the regclass of the opcode has VGPRs (if it doesn't, we need to move-to-VALU, i.e. add to worklist). If it doesn't exist, IMO we should just add to the worklist unconditionally, and handle the fallout (if any) in the main moveToVALU method. That way, the more complex logic is centered in one place.

I'd appreciate if you could help cleanup the technical debt here on those points...

[arsenm]

getOpRegClass behaves the way it does because otherwise it will fail on any generic or variadic instructions, e.g. copy or a call instruction

[sheredom]

Removed canReadVGPR as it had only a single callsite.

[sheredom]

@nhaehnle I've removed canReadVGPR as it only had the single callsite - but given @arsenm's comment I did not change getOpRegClass. I also don't feel super comfortable doing the change you suggested to addUsersToMoveToVALUWorklist as I'm worried that there will be non-LLVM-tested paths that I could trip up on with ease. I'd rather ship the commit as is if y'all are ok with it.

[arsenm]

LGTM but I know hardly anything about WWM

test/CodeGen/AMDGPU/fix-wwm-vgpr-copy.ll

5

Move to -mtriple and remove the triple/datalayout

[sheredom]

Fixed review comments.

[tpr]

LGTM.

I was thinking it is a bit odd that we're having to handle WQM and WWM in this way even though they are target instructions. However I think it is ok because they act like the non-target COPY instruction in that they do not specify a register class for operands.

lib/Target/AMDGPU/SIInstrInfo.h

if (MO.isReg()) {
                                      getRegClass(MO.getReg()), SubReg)) >= 32 &&
                "Sub-dword subregs are not supported");
         return RI.getSubRegIndexLaneMask(SubReg).getNumLanes() * 4;
       }
     }
     return RI.getRegSizeInBits(*getOpRegClass(MI, OpNo)) / 8;
   }
 
-  /// \returns true if it is legal for the operand at index \p OpNo
-  /// to read a VGPR.
-  bool canReadVGPR(const MachineInstr &MI, unsigned OpNo) const;
-
   /// Legalize the \p OpIndex operand of this instruction by inserting
   /// a MOV.  For example:
   /// ADD_I32_e32 VGPR0, 15
   /// to
   /// MOV VGPR1, 15
   /// ADD_I32_e32 VGPR0, VGPR1
   ///
   /// If the operand being legalized is a register, then a COPY will be used

lib/Target/AMDGPU/SIInstrInfo.cpp

if (TargetRegisterInfo::isVirtualRegister(Reg))
       return MRI.getRegClass(Reg);
     return RI.getPhysRegClass(Reg);
   }
 
   unsigned RCID = Desc.OpInfo[OpNo].RegClass;
   return RI.getRegClass(RCID);
 }
 
-bool SIInstrInfo::canReadVGPR(const MachineInstr &MI, unsigned OpNo) const {
-  switch (MI.getOpcode()) {
-  case AMDGPU::COPY:
-  case AMDGPU::REG_SEQUENCE:
-  case AMDGPU::PHI:
-  case AMDGPU::INSERT_SUBREG:
-    return RI.hasVGPRs(getOpRegClass(MI, 0));
-  default:
-    return RI.hasVGPRs(getOpRegClass(MI, OpNo));
-  }
-}
-
 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;
   const TargetRegisterClass *RC = RI.getRegClass(RCID);
   unsigned Opcode = AMDGPU::V_MOV_B32_e32;

 
 void SIInstrInfo::addUsersToMoveToVALUWorklist(
   unsigned DstReg,
   MachineRegisterInfo &MRI,
   SetVectorType &Worklist) const {
   for (MachineRegisterInfo::use_iterator I = MRI.use_begin(DstReg),
          E = MRI.use_end(); I != E;) {
     MachineInstr &UseMI = *I->getParent();
-    if (!canReadVGPR(UseMI, I.getOperandNo())) {
+
+    unsigned OpNo = 0;
+
+    switch (UseMI.getOpcode()) {
+    case AMDGPU::COPY:
+    case AMDGPU::WQM:
+    case AMDGPU::WWM:
+    case AMDGPU::REG_SEQUENCE:
+    case AMDGPU::PHI:
+    case AMDGPU::INSERT_SUBREG:
+      break;
+    default:
+      OpNo = I.getOperandNo();
+      break;
+    }
+
+    if (!RI.hasVGPRs(getOpRegClass(UseMI, OpNo))) {
       Worklist.insert(&UseMI);
 
       do {
         ++I;
       } while (I != E && I->getParent() == &UseMI);
     } else {
       ++I;
     }

test/CodeGen/AMDGPU/fix-wwm-vgpr-copy.ll

+; RUN: llc -mtriple=amdgcn--amdpal -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+
+declare i64 @llvm.amdgcn.icmp.i32(i32, i32, i32) #0
+declare i32 @llvm.amdgcn.set.inactive.i32(i32, i32) #0
+declare i32 @llvm.amdgcn.wwm.i32(i32) #1
+declare void @llvm.amdgcn.tbuffer.store.f32(float, <4 x i32>, i32, i32, i32, i32, i32, i32, i1, i1) #2
+declare void @llvm.amdgcn.exp.f32(i32, i32, float, float, float, float, i1, i1) #2
+
+define amdgpu_hs void @foo(i32 inreg %arg, <4 x i32> inreg %buffer) {
+entry:
+  br label %work
+
+bb42:
+  br label %bb602
+
+bb602:
+  %tmp603 = phi i32 [ 0, %bb42 ], [ 1, %work ]
+  %tmp607 = icmp eq i32 %tmp603, %tmp1196
+  br i1 %tmp607, label %bb49, label %bb54
+
+bb49:
+  tail call void @llvm.amdgcn.tbuffer.store.f32(float 1.000000e+00, <4 x i32> %buffer, i32 0, i32 1, i32 1, i32 4, i32 4, i32 7, i1 true, i1 false) #7
+  ret void
+
+bb54:
+  ret void
+
+work:
+; GCN: s_not_b64 exec, exec
+; GCN: v_mov_b32_e32 v[[tmp1189:[0-9]+]], 1
+; GCN: s_not_b64 exec, exec
+  %tmp1189 = tail call i32 @llvm.amdgcn.set.inactive.i32(i32 4, i32 1)
+
+; GCN: s_or_saveexec_b64 s{{\[}}[[LO:[0-9]+]]:[[HI:[0-9]+]]{{\]}}, -1
+; GCN: v_lshlrev_b32_e32 v[[tmp1191:[0-9]+]], 2, v[[tmp1189]]
+  %tmp1191 = mul i32 %tmp1189, 4
+
+; GCN: s_mov_b64 exec, s{{\[}}[[LO]]:[[HI]]{{\]}}
+  %tmp1196 = tail call i32 @llvm.amdgcn.wwm.i32(i32 %tmp1191)
+
+  %tmp34 = icmp eq i32 %arg, 0
+  br i1 %tmp34, label %bb602, label %bb42
+}
+
+attributes #0 = { convergent nounwind readnone }
+attributes #1 = { nounwind readnone speculatable }
+attributes #2 = { nounwind writeonly }