[AMDGPU][GFX11] Refactor VOPD operands handling

[dp]

Summary

Correct VOPD ComponentLayout to explicitly use ComponentProps of previous component (if any). Hopefully this will make code more clear.

This is actually an NFC change. It only affects one existing test for error position.

[Joe_Nash]

This looks like a reasonable refactor. Why does it change the error position?

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

8528

Can we rename SrcIdx to something like MCSrcIdx? So that there is no confusion with the ParsedSrcIndex returned by getParsedSrcIndex.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

532

I think there should be a documentation comment for this function.

[dp]

Why does it change the error position?

Before this change getParsedSrcIndex handled tied operands as if they were parsed and returned dst index for tied operands:

unsigned getParsedSrcIndex(unsigned SrcIdx, bool ComponentHasSrc2Acc) const {
  if (ComponentHasSrc2Acc && SrcIdx == (MAX_SRC_NUM - 1))
    return getParsedDstIndex();
  ...
}

This patch corrects handling of parsed operands: now they don't include tied ones.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

8528

I agree that there is some confusion with naming. It should be clear when the code operates with indices to opcode operands, indices to parsed operands and indices to MC operands.

But MC prefix would suggest iteration over MCInst operands, would it not? Maybe OpSrcIdx would be a better choice?

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

545

Another problem with naming is here: getSrcIndex(SrcIdx) looks awful to me. How about renaming getSrc* (and other similar functions) to getMCSrc*?

[Joe_Nash]

In D137952#3925471, @dp wrote:

Why does it change the error position?

Before this change getParsedSrcIndex handled tied operands as if they were parsed and returned dst index for tied operands:

unsigned getParsedSrcIndex(unsigned SrcIdx, bool ComponentHasSrc2Acc) const {
  if (ComponentHasSrc2Acc && SrcIdx == (MAX_SRC_NUM - 1))
    return getParsedDstIndex();
  ...
}

This patch corrects handling of parsed operands: now they don't include tied ones.

Thanks for the explanation. Then this patch is nicely beneficial, but perhaps not NFC.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

8528

Now I see that it does not iterate over MCInst operands. So clearly it is confusing :).
SrcIdx is an index into parsed src operands which is a subset of parsed operands.
So maybe these renames are most accurate:

getParsedSrcIndex -> getParsedOpIdxFromSrcIdx
SrcIdx -> CombinedSrcIdx
getParsedDstIndex -> getParsedOpIdxOfDst

In getRegIndicies
I think SrcIdx is acutally ComponentSrcIdx
Renames:
SrcIdx -> ComponentSrcIdx
getSrcIndex -> getCombinedMCSrcIdxFromComponentSrcIdx

obviously this is getting really verbose, but that's probably better than confusingly named.

[dp]

Add a comment describing InstInfo::getRegIndices.

[dp]

I suggest doing renaming in a separate patch because it is a separate problem introduced before this change.

[dp]

Rebase.

[Joe_Nash]

In D137952#3927187, @dp wrote:

I suggest doing renaming in a separate patch because it is a separate problem introduced before this change.

Ok.

This patch LGTM

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

bool AMDGPUAsmParser::validateVOPDRegBankConstraints(
 
   auto getVRegIdx = [&](unsigned, unsigned OperandIdx) {
     const MCOperand &Opr = Inst.getOperand(OperandIdx);
     return (Opr.isReg() && !isSGPR(mc2PseudoReg(Opr.getReg()), TRI))
                ? Opr.getReg()
                : MCRegister::NoRegister;
   };
 
-  auto InstInfo = getVOPDInstInfo(Opcode, &MII);
+  const auto &InstInfo = getVOPDInstInfo(Opcode, &MII);
   auto InvalidOperandInfo = InstInfo.getInvalidOperandIndex(getVRegIdx);
   if (!InvalidOperandInfo)
     return true;
 
   auto OprIdx = *InvalidOperandInfo;
   auto ParsedIdx = std::max(InstInfo[VOPD::X].getParsedOperandIndex(OprIdx),
                             InstInfo[VOPD::Y].getParsedOperandIndex(OprIdx));
   assert(ParsedIdx > 0 && ParsedIdx < Operands.size());
void AMDGPUAsmParser::cvtVOPD(MCInst &Inst, const OperandVector &Operands) {
   //   dstX, dstY, src0X [, other OpX operands], src0Y [, other OpY operands]
 
   for (auto CompIdx : VOPD::COMPONENTS) {
     addOp(InstInfo[CompIdx].getParsedDstIndex());
   }
 
   for (auto CompIdx : VOPD::COMPONENTS) {
     const auto &CInfo = InstInfo[CompIdx];
-    bool CompHasSrc2Acc = CInfo.hasSrc2Acc();
-    auto SrcOperandsNum = InstInfo[CompIdx].getSrcOperandsNum();
-    for (unsigned SrcIdx = 0; SrcIdx < SrcOperandsNum; ++SrcIdx) {
-      addOp(CInfo.getParsedSrcIndex(SrcIdx, CompHasSrc2Acc));
-    }
+    auto ParsedSrcOperandsNum = InstInfo[CompIdx].getParsedSrcOperandsNum();
+    for (unsigned SrcIdx = 0; SrcIdx < ParsedSrcOperandsNum; ++SrcIdx)
+      addOp(CInfo.getParsedSrcIndex(SrcIdx));
+    if (CInfo.hasSrc2Acc())
+      addOp(CInfo.getParsedDstIndex());
   }
 }
 
 //===----------------------------------------------------------------------===//
 // dpp
 //===----------------------------------------------------------------------===//
 
 bool AMDGPUOperand::isDPP8() const {

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

 
 // Number of VGPR banks per VOPD component operand.
 constexpr unsigned BANKS_NUM[] = {2, 4, 4, 2};
 
 enum ComponentIndex : unsigned { X = 0, Y = 1 };
 constexpr unsigned COMPONENTS[] = {ComponentIndex::X, ComponentIndex::Y};
 constexpr unsigned COMPONENTS_NUM = 2;
 
+// Properties of VOPD components.
+class ComponentProps {
+private:
+  unsigned SrcOperandsNum = 0;
+  Optional<unsigned> MandatoryLiteralIdx;
+  bool HasSrc2Acc = false;
+
+public:
+  ComponentProps() = default;
+  ComponentProps(const MCInstrDesc &OpDesc);
+
+  unsigned getSrcOperandsNum() const { return SrcOperandsNum; }
+  unsigned getParsedSrcOperandsNum() const {
+    return SrcOperandsNum - HasSrc2Acc;
+  }
+  bool hasMandatoryLiteral() const { return MandatoryLiteralIdx.has_value(); }
+  unsigned getMandatoryLiteralIndex() const {
+    assert(hasMandatoryLiteral());
+    return *MandatoryLiteralIdx;
+  }
+  bool hasRegSrcOperand(unsigned SrcIdx) const {
+    assert(SrcIdx < Component::MAX_SRC_NUM);
+    return SrcOperandsNum > SrcIdx && !hasMandatoryLiteralAt(SrcIdx);
+  }
+  bool hasSrc2Acc() const { return HasSrc2Acc; }
+
+private:
+  bool hasMandatoryLiteralAt(unsigned SrcIdx) const {
+    assert(SrcIdx < Component::MAX_SRC_NUM);
+    return hasMandatoryLiteral() &&
+           *MandatoryLiteralIdx == Component::DST_NUM + SrcIdx;
+  }
+};
+
 enum ComponentKind : unsigned {
   SINGLE = 0,  // A single VOP1 or VOP2 instruction which may be used in VOPD.
   COMPONENT_X, // A VOPD instruction, X component.
   COMPONENT_Y, // A VOPD instruction, Y component.
   MAX = COMPONENT_Y
 };
 
 // Location of operands in a MachineInstr/MCInst
 // and position of operands in parsed operands array.
 class ComponentLayout {
 private:
   // Regular MachineInstr/MCInst operands are ordered as follows:
   //   dst, src0 [, other src operands]
   // VOPD MachineInstr/MCInst operands are ordered as follows:
   //   dstX, dstY, src0X [, other OpX operands], src0Y [, other OpY operands]
   // Each ComponentKind has operand indices defined below.
   static constexpr unsigned MC_DST_IDX[] = {0, 0, 1};
-  static constexpr unsigned FIRST_MC_SRC_IDX[] = {1, 2, 2 /* + OpXSrcNum */};
+  static constexpr unsigned FIRST_MC_SRC_IDX[] = {1, 2, 2 /* + OpX.SrcNum */};
 
   // Parsed operands of regular instructions are ordered as follows:
   //   Mnemo dst src0 [vsrc1 ...]
   // Parsed VOPD operands are ordered as follows:
   //   OpXMnemo dstX src0X [vsrc1X|imm vsrc1X|vsrc1X imm] '::'
   //   OpYMnemo dstY src0Y [vsrc1Y|imm vsrc1Y|vsrc1Y imm]
   // Each ComponentKind has operand indices defined below.
   static constexpr unsigned PARSED_DST_IDX[] = {1, 1,
-                                                4 /* + ParsedOpXSrcNum */};
+                                                4 /* + OpX.ParsedSrcNum */};
   static constexpr unsigned FIRST_PARSED_SRC_IDX[] = {
-      2, 2, 5 /* + ParsedOpXSrcNum */};
+      2, 2, 5 /* + OpX.ParsedSrcNum */};
 
 private:
-  ComponentKind Kind;
-  unsigned OpXSrcNum;
-  unsigned ParsedOpXSrcNum;
+  const ComponentKind Kind;
+  const ComponentProps PrevOp;
 
 public:
-  ComponentLayout(ComponentKind Kind = ComponentKind::SINGLE,
-                  unsigned OpXSrcNum = 0, unsigned ParsedOpXSrcNum = 0)
-      : Kind(Kind), OpXSrcNum(OpXSrcNum), ParsedOpXSrcNum(ParsedOpXSrcNum) {
-    assert(Kind <= ComponentKind::MAX);
-    assert((Kind == ComponentKind::COMPONENT_Y) == (OpXSrcNum > 0));
+  // Create layout for COMPONENT_X or SINGLE component
+  ComponentLayout(ComponentKind Kind) : Kind(Kind) {
+    assert(Kind == ComponentKind::SINGLE || Kind == ComponentKind::COMPONENT_X);
   }
 
+  // Create layout for COMPONENT_Y which depends on COMPONENT_X layout
+  ComponentLayout(const ComponentProps &OpXProps)
+      : Kind(ComponentKind::COMPONENT_Y), PrevOp(OpXProps) {}
+
 public:
   unsigned getDstIndex() const { return MC_DST_IDX[Kind]; }
   unsigned getSrcIndex(unsigned SrcIdx) const {
     assert(SrcIdx < Component::MAX_SRC_NUM);
-    return FIRST_MC_SRC_IDX[Kind] + OpXSrcNum + SrcIdx;
+    return FIRST_MC_SRC_IDX[Kind] + getPrevOpSrcNum() + SrcIdx;
   }
 
   unsigned getParsedDstIndex() const {
-    return PARSED_DST_IDX[Kind] + ParsedOpXSrcNum;
+    return PARSED_DST_IDX[Kind] + getPrevOpParsedSrcNum();
   }
-  unsigned getParsedSrcIndex(unsigned SrcIdx, bool ComponentHasSrc2Acc) const {
+  unsigned getParsedSrcIndex(unsigned SrcIdx) const {
     assert(SrcIdx < Component::MAX_SRC_NUM);
-    // FMAC and DOT2C have a src2 operand on the MCInst but
-    // not on the asm representation. src2 is tied to dst.
-    if (ComponentHasSrc2Acc && SrcIdx == (MAX_SRC_NUM - 1))
-      return getParsedDstIndex();
-    return FIRST_PARSED_SRC_IDX[Kind] + ParsedOpXSrcNum + SrcIdx;
+    return FIRST_PARSED_SRC_IDX[Kind] + getPrevOpParsedSrcNum() + SrcIdx;
   }
-};
 
-// Properties of VOPD components.
-class ComponentProps {
 private:
-  unsigned SrcOperandsNum;
-  Optional<unsigned> MandatoryLiteralIdx;
-  bool HasSrc2Acc;
-
-public:
-  ComponentProps(const MCInstrDesc &OpDesc);
-
-  unsigned getSrcOperandsNum() const { return SrcOperandsNum; }
-  bool hasMandatoryLiteral() const { return MandatoryLiteralIdx.has_value(); }
-  unsigned getMandatoryLiteralIndex() const {
-    assert(hasMandatoryLiteral());
-    return *MandatoryLiteralIdx;
-  }
-  bool hasRegularSrcOperand(unsigned SrcIdx) const {
-    assert(SrcIdx < Component::MAX_SRC_NUM);
-    return SrcOperandsNum > SrcIdx && !hasMandatoryLiteralAt(SrcIdx);
-  }
-  bool hasSrc2Acc() const { return HasSrc2Acc; }
-
-private:
-  bool hasMandatoryLiteralAt(unsigned SrcIdx) const {
-    assert(SrcIdx < Component::MAX_SRC_NUM);
-    return hasMandatoryLiteral() &&
-           *MandatoryLiteralIdx == Component::DST_NUM + SrcIdx;
+  unsigned getPrevOpSrcNum() const { return PrevOp.getSrcOperandsNum(); }
+  unsigned getPrevOpParsedSrcNum() const {
+    return PrevOp.getParsedSrcOperandsNum();
   }
 };
 
 // Layout and properties of VOPD components.
 class ComponentInfo : public ComponentLayout, public ComponentProps {
 public:
+  // Create ComponentInfo for COMPONENT_X or SINGLE component
+  ComponentInfo(const MCInstrDesc &OpDesc,
+                ComponentKind Kind = ComponentKind::SINGLE)
+      : ComponentLayout(Kind), ComponentProps(OpDesc) {}
+
+  // Create ComponentInfo for COMPONENT_Y which depends on COMPONENT_X layout
   ComponentInfo(const MCInstrDesc &OpDesc,
-                ComponentKind Kind = ComponentKind::SINGLE,
-                unsigned OpXSrcNum = 0, unsigned ParsedOpXSrcNum = 0)
-      : ComponentLayout(Kind, OpXSrcNum, ParsedOpXSrcNum),
-        ComponentProps(OpDesc) {}
+                const ComponentProps &OpXProps)
+      : ComponentLayout(OpXProps), ComponentProps(OpDesc) {}
 
   // Map MC operand index to parsed operand index.
   // Return 0 if the specified operand does not exist.
   unsigned getParsedOperandIndex(unsigned OprIdx) const;
 };
 
 // Properties of VOPD instructions.
 class InstInfo {

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

 
 unsigned ComponentInfo::getParsedOperandIndex(unsigned OprIdx) const {
   assert(OprIdx < Component::MAX_OPR_NUM);
 
   if (OprIdx == Component::DST)
     return getParsedDstIndex();
 
   auto SrcIdx = OprIdx - Component::DST_NUM;
-  if (SrcIdx < getSrcOperandsNum())
-    return getParsedSrcIndex(SrcIdx, hasSrc2Acc());
+  if (SrcIdx < getParsedSrcOperandsNum())
+    return getParsedSrcIndex(SrcIdx);
 
   // The specified operand does not exist.
   return 0;
 }
 
 Optional<unsigned> InstInfo::getInvalidOperandIndex(
     std::function<unsigned(unsigned, unsigned)> GetRegIdx) const {
 
   auto OpXRegs = getRegIndices(ComponentIndex::X, GetRegIdx);
   auto OpYRegs = getRegIndices(ComponentIndex::Y, GetRegIdx);
 
   for (unsigned OprIdx = 0; OprIdx < Component::MAX_OPR_NUM; ++OprIdx) {
     unsigned BanksNum = BANKS_NUM[OprIdx];
     if (OpXRegs[OprIdx] && OpYRegs[OprIdx] &&
         (OpXRegs[OprIdx] % BanksNum == OpYRegs[OprIdx] % BanksNum))
       return OprIdx;
   }
 
   return {};
 }
 
+// Return an array of VGPR registers [DST,SRC0,SRC1,SRC2] used
+// by the specified component. If an operand is unused
+// or is not a VGPR, the corresponding value is 0.
+//
+// GetRegIdx(Component, OperandIdx) must return a VGPR register index
+// for the specified component and operand. The callback must return 0
+// if the operand is not a register or not a VGPR.
 InstInfo::RegIndices InstInfo::getRegIndices(
     unsigned ComponentIdx,
     std::function<unsigned(unsigned, unsigned)> GetRegIdx) const {
   assert(ComponentIdx < COMPONENTS_NUM);
 
   auto Comp = CompInfo[ComponentIdx];
+  InstInfo::RegIndices RegIndices;
 
-  unsigned DstReg = GetRegIdx(ComponentIdx, Comp.getDstIndex());
-  unsigned Src0Reg = GetRegIdx(ComponentIdx, Comp.getSrcIndex(0));
-
-  unsigned Src1Reg = 0;
-  if (Comp.hasRegularSrcOperand(1))
-    Src1Reg = GetRegIdx(ComponentIdx, Comp.getSrcIndex(1));
-
-  unsigned Src2Reg = 0;
-  if (Comp.hasRegularSrcOperand(2))
-    Src2Reg = GetRegIdx(ComponentIdx, Comp.getSrcIndex(2));
+  RegIndices[DST] = GetRegIdx(ComponentIdx, Comp.getDstIndex());
 
-  return {DstReg, Src0Reg, Src1Reg, Src2Reg};
+  for (unsigned OprIdx : {SRC0, SRC1, SRC2}) {
+    unsigned SrcIdx = OprIdx - DST_NUM;
+    RegIndices[OprIdx] = Comp.hasRegSrcOperand(SrcIdx)
+                             ? GetRegIdx(ComponentIdx, Comp.getSrcIndex(SrcIdx))
+                             : 0;
+  }
+  return RegIndices;
 }
 
 } // namespace VOPD
 
 VOPD::InstInfo getVOPDInstInfo(const MCInstrDesc &OpX, const MCInstrDesc &OpY) {
   return VOPD::InstInfo(OpX, OpY);
 }
 
 VOPD::InstInfo getVOPDInstInfo(unsigned VOPDOpcode,
                                const MCInstrInfo *InstrInfo) {
   auto [OpX, OpY] = getVOPDComponents(VOPDOpcode);
   const auto &OpXDesc = InstrInfo->get(OpX);
   const auto &OpYDesc = InstrInfo->get(OpY);
   VOPD::ComponentInfo OpXInfo(OpXDesc, VOPD::ComponentKind::COMPONENT_X);
-  VOPD::ComponentInfo OpYInfo(
-      OpYDesc, VOPD::ComponentKind::COMPONENT_Y, OpXInfo.getSrcOperandsNum(),
-      OpXInfo.getSrcOperandsNum() - OpXInfo.hasSrc2Acc());
+  VOPD::ComponentInfo OpYInfo(OpYDesc, OpXInfo);
   return VOPD::InstInfo(OpXInfo, OpYInfo);
 }
 
 namespace IsaInfo {
 
 AMDGPUTargetID::AMDGPUTargetID(const MCSubtargetInfo &STI)
     : STI(STI), XnackSetting(TargetIDSetting::Any),
       SramEccSetting(TargetIDSetting::Any) {

llvm/test/MC/AMDGPU/gfx11_asm_vopd_err.s

 v_dual_fmac_f32     v7, v1, v2                   :: v_dual_fmamk_f32      v6, v2, 0xaf123456, v3
 // GFX11: error: src2 operands must use different VGPR banks
 // GFX11-NEXT:{{^}}v_dual_fmac_f32     v7, v1, v2                   :: v_dual_fmamk_f32      v6, v2, 0xaf123456, v3
 // GFX11-NEXT:{{^}}                                                                                              ^
 
 v_dual_fmamk_f32    v6, v1, 0xaf123456, v3       :: v_dual_fmac_f32       v5, v2, v3
 // GFX11: error: src2 operands must use different VGPR banks
 // GFX11-NEXT:{{^}}v_dual_fmamk_f32    v6, v1, 0xaf123456, v3       :: v_dual_fmac_f32       v5, v2, v3
-// GFX11-NEXT:{{^}}                                                                          ^
+// GFX11-NEXT:{{^}}                                        ^
 
 //===----------------------------------------------------------------------===//
 // Check invalid VOPD syntax.
 //===----------------------------------------------------------------------===//
 
 v_dual_fmamk_f32    v6, v1, 0xaf123456, v2       : : v_dual_fmac_f32       v5, v2, v3
 // GFX11: error: unknown token in expression
 // GFX11-NEXT:{{^}}v_dual_fmamk_f32    v6, v1, 0xaf123456, v2       : : v_dual_fmac_f32       v5, v2, v3