[AMDGPU/MemOpsCluster] Code clean-up around accessing of memory operand width

[hsmhsm]

Summary

Clean-up the width computing logic given a memory operand, and re-arrange code to avoid
code duplication.

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.h

839

Losing the units in the name is a regression. I would also pass in MRI explicitly rather than getting it from the parent function

[hsmhsm]

llvm/lib/Target/AMDGPU/SIInstrInfo.h

839

The function getOperandSizeInBytes() is not something existing for quite some time, It was just added by me in my previous patch https://reviews.llvm.org/D80545. Then, I saw few already existing functions with the name getOpSize to get operand size in bytes (see ones just above this function). So, I favored uniformity here, and added another required overloaded function here. In that since it is not a regression.

If we do not get MRI from the parent here, then, caller of the getOpSize() has to do it. Is not it good to get it one place rather calling getParents repeatedly from caller side in multiple places?

[hsmhsm]

Have taken care of second review comment by Matt, and first one, I think is not necessary to take care.

[hsmhsm]

llvm/lib/Target/AMDGPU/SIInstrInfo.h

839

Anyway, second comment is taken care, first one, I think is not necessary.

[foad]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

276–277

MO is not a very descriptive name. How about DataOp (or DataOpIdx when you switch to using getNamedOperandIdx)?

llvm/lib/Target/AMDGPU/SIInstrInfo.h

839

There's no need for this new overload if you use getNamedOperandIdx instead of getNamedOperand and pass the index into getOpSize(MI, OpNo).

[hsmhsm]

Have taken care of review comments by Jay.

llvm/lib/Target/AMDGPU/SIInstrInfo.h

public:
 
   bool isReallyTriviallyReMaterializable(const MachineInstr &MI,
                                          AAResults *AA) const override;
 
   bool areLoadsFromSameBasePtr(SDNode *Load1, SDNode *Load2,
                                int64_t &Offset1,
                                int64_t &Offset2) const override;
 
-  unsigned getOperandSizeInBytes(const MachineInstr &LdSt,
-                                 const MachineOperand *MOp) const;
-
   bool getMemOperandsWithOffsetWidth(
       const MachineInstr &LdSt,
       SmallVectorImpl<const MachineOperand *> &BaseOps, int64_t &Offset,
       bool &OffsetIsScalable, unsigned &Width,
       const TargetRegisterInfo *TRI) const final;
 
   bool shouldClusterMemOps(ArrayRef<const MachineOperand *> BaseOps1,
                            ArrayRef<const MachineOperand *> BaseOps2,
unsigned getOpSize(const MachineInstr &MI, unsigned OpNo) const {
     if (MO.isReg()) {
       if (unsigned SubReg = MO.getSubReg()) {
         return RI.getSubRegIdxSize(SubReg) / 8;
       }
     }
     return RI.getRegSizeInBits(*getOpRegClass(MI, OpNo)) / 8;
   }
 
   /// 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
   /// instead of MOV.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

static bool isStride64(unsigned Opc) {
   case AMDGPU::DS_WRITE2ST64_B32:
   case AMDGPU::DS_WRITE2ST64_B64:
     return true;
   default:
     return false;
   }
 }
 
-unsigned SIInstrInfo::getOperandSizeInBytes(const MachineInstr &LdSt,
-                                            const MachineOperand *MOp) const {
-  assert(MOp && "Unexpected null machine operand!");
-  const MachineRegisterInfo &MRI = LdSt.getParent()->getParent()->getRegInfo();
-  const Register Reg = MOp->getReg();
-  const TargetRegisterClass *DstRC = Register::isVirtualRegister(Reg)
-                                         ? MRI.getRegClass(Reg)
-                                         : RI.getPhysRegClass(Reg);
-  return (RI.getRegSizeInBits(*DstRC) / 8);
-}
-
 bool SIInstrInfo::getMemOperandsWithOffsetWidth(
     const MachineInstr &LdSt, SmallVectorImpl<const MachineOperand *> &BaseOps,
     int64_t &Offset, bool &OffsetIsScalable, unsigned &Width,
     const TargetRegisterInfo *TRI) const {
   if (!LdSt.mayLoadOrStore())
     return false;
 
   unsigned Opc = LdSt.getOpcode();
   OffsetIsScalable = false;
-  const MachineOperand *BaseOp, *OffsetOp, *MOp;
+  const MachineOperand *BaseOp, *OffsetOp;
+  int DataOpIdx;
 
   if (isDS(LdSt)) {
     BaseOp = getNamedOperand(LdSt, AMDGPU::OpName::addr);
     OffsetOp = getNamedOperand(LdSt, AMDGPU::OpName::offset);
     if (OffsetOp) {
       // Normal, single offset LDS instruction.
       if (!BaseOp) {
         // DS_CONSUME/DS_APPEND use M0 for the base address.
         // TODO: find the implicit use operand for M0 and use that as BaseOp?
         return false;
       }
       BaseOps.push_back(BaseOp);
       Offset = OffsetOp->getImm();
       // Get appropriate operand, and compute width accordingly.
-      MOp = getNamedOperand(LdSt, AMDGPU::OpName::vdst);
-      if (!MOp)
-        MOp = getNamedOperand(LdSt, AMDGPU::OpName::data0);
-      Width = getOperandSizeInBytes(LdSt, MOp);
+      DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst);
+      if (DataOpIdx == -1)
+        DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::data0);
+      Width = getOpSize(LdSt, DataOpIdx);
     } else {
       // The 2 offset instructions use offset0 and offset1 instead. We can treat
       // these as a load with a single offset if the 2 offsets are consecutive.
       // We will use this for some partially aligned loads.
       const MachineOperand *Offset0Op =
           getNamedOperand(LdSt, AMDGPU::OpName::offset0);
       const MachineOperand *Offset1Op =
           getNamedOperand(LdSt, AMDGPU::OpName::offset1);
if (OffsetOp) {
       }
 
       if (isStride64(Opc))
         EltSize *= 64;
 
       BaseOps.push_back(BaseOp);
       Offset = EltSize * Offset0;
       // Get appropriate operand(s), and compute width accordingly.
-      MOp = getNamedOperand(LdSt, AMDGPU::OpName::vdst);
-      if (!MOp) {
-        MOp = getNamedOperand(LdSt, AMDGPU::OpName::data0);
-        Width = getOperandSizeInBytes(LdSt, MOp);
-        MOp = getNamedOperand(LdSt, AMDGPU::OpName::data1);
-        Width += getOperandSizeInBytes(LdSt, MOp);
+      DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst);
+      if (DataOpIdx == -1) {
+        DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::data0);
+        Width = getOpSize(LdSt, DataOpIdx);
+        DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::data1);
+        Width += getOpSize(LdSt, DataOpIdx);
       } else {
-        Width = getOperandSizeInBytes(LdSt, MOp);
+        Width = getOpSize(LdSt, DataOpIdx);
       }
     }
     return true;
   }
 
   if (isMUBUF(LdSt) || isMTBUF(LdSt)) {
     const MachineOperand *SOffset = getNamedOperand(LdSt, AMDGPU::OpName::soffset);
     if (SOffset && SOffset->isReg()) {
if (SOffset && SOffset->isReg()) {
       if (RSrc->getReg() != MFI->getScratchRSrcReg())
         return false;
 
       const MachineOperand *OffsetImm =
         getNamedOperand(LdSt, AMDGPU::OpName::offset);
       BaseOps.push_back(RSrc);
       BaseOps.push_back(SOffset);
       Offset = OffsetImm->getImm();
-      // Get appropriate operand, and compute width accordingly.
-      MOp = getNamedOperand(LdSt, AMDGPU::OpName::vdst);
-      if (!MOp)
-        MOp = getNamedOperand(LdSt, AMDGPU::OpName::vdata);
-      Width = getOperandSizeInBytes(LdSt, MOp);
-      return true;
-    }
-
+    } else {
       BaseOp = getNamedOperand(LdSt, AMDGPU::OpName::srsrc);
       if (!BaseOp) // e.g. BUFFER_WBINVL1_VOL
         return false;
       BaseOps.push_back(BaseOp);
 
       BaseOp = getNamedOperand(LdSt, AMDGPU::OpName::vaddr);
       if (BaseOp)
         BaseOps.push_back(BaseOp);
 
       const MachineOperand *OffsetImm =
           getNamedOperand(LdSt, AMDGPU::OpName::offset);
       Offset = OffsetImm->getImm();
       if (SOffset) // soffset can be an inline immediate.
         Offset += SOffset->getImm();
+    }
     // Get appropriate operand, and compute width accordingly.
-    MOp = getNamedOperand(LdSt, AMDGPU::OpName::vdst);
-    if (!MOp)
-      MOp = getNamedOperand(LdSt, AMDGPU::OpName::vdata);
-    Width = getOperandSizeInBytes(LdSt, MOp);
+    DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst);
+    if (DataOpIdx == -1)
+      DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdata);
+    Width = getOpSize(LdSt, DataOpIdx);
     return true;
   }
 
   if (isSMRD(LdSt)) {
     BaseOp = getNamedOperand(LdSt, AMDGPU::OpName::sbase);
     if (!BaseOp) // e.g. S_MEMTIME
       return false;
     BaseOps.push_back(BaseOp);
     OffsetOp = getNamedOperand(LdSt, AMDGPU::OpName::offset);
     Offset = OffsetOp ? OffsetOp->getImm() : 0;
     // Get appropriate operand, and compute width accordingly.
-    MOp = getNamedOperand(LdSt, AMDGPU::OpName::sdst);
-    Width = getOperandSizeInBytes(LdSt, MOp);
+    DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::sdst);
+    Width = getOpSize(LdSt, DataOpIdx);
     return true;
   }
 
   if (isFLAT(LdSt)) {
     // Instructions have either vaddr or saddr or both.
     BaseOp = getNamedOperand(LdSt, AMDGPU::OpName::vaddr);
     if (BaseOp)
       BaseOps.push_back(BaseOp);
     BaseOp = getNamedOperand(LdSt, AMDGPU::OpName::saddr);
     if (BaseOp)
       BaseOps.push_back(BaseOp);
     Offset = getNamedOperand(LdSt, AMDGPU::OpName::offset)->getImm();
     // Get appropriate operand, and compute width accordingly.
-    MOp = getNamedOperand(LdSt, AMDGPU::OpName::vdst);
-    if (!MOp)
-      MOp = getNamedOperand(LdSt, AMDGPU::OpName::vdata);
-    Width = getOperandSizeInBytes(LdSt, MOp);
+    DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst);
+    if (DataOpIdx == -1)
+      DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdata);
+    Width = getOpSize(LdSt, DataOpIdx);
     return true;
   }
 
   return false;
 }
 
 static bool
 memOpsHaveSameBaseOperands(ArrayRef<const MachineOperand *> BaseOps1,