[PATCH] D134960: [AMDGPU][MC][GFX11] Add VOPD VGPR bank access validation

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash accepted this revision.
Joe_Nash added a comment.
This revision is now accepted and ready to land.

Except for a few nits and questions, this patch LGTM. The simplification in AsmParser is quite elegant. @foad please take a look at the interface design in AMDGPUBaseInfo.h



================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:1667
   bool validateConstantBusLimitations(const MCInst &Inst, const OperandVector &Operands);
+  bool validateVOPDOperands(const MCInst &Inst, const OperandVector &Operands);
   bool validateIntClampSupported(const MCInst &Inst);
----------------
This is checking reg bank constraints only, so rename to validateVOPDRegBankConstraints. We check constant bus limits elsewhere.


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:3589
+    const MCOperand &Opr = Inst.getOperand(OperandIdx);
+    return (Opr.isReg() && VGPR32.contains(Opr.getReg()))
+               ? Opr.getReg()
----------------
VGPR32 may be an issue for V_DUAL_DOT2ACC_F32_F16 as True16 work progresses, can you make it any VGPR?


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:507
+
+  return 0;
+}
----------------
When is 0 the expected result? Can you add a comment?


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:552
+VOPD::InstInfo getVOPDInstInfo(const MCInstrDesc &OpX, const MCInstrDesc &OpY) {
+  return {OpX, OpY};
+}
----------------
Is implicitly converting the return value by constructing good c++ style? I have no idea if it is or not, but it took me a minute to realize it happened.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:544
+  static constexpr unsigned MC_DST_IDX[] = {0, 0, 1};
+  static constexpr unsigned MC_SRC_IDX[] = {1, 2, 2 /* + OpXSrcNum */};
+
----------------
Name this FIRST_MC_SRC_IDX ?


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:553
+  static constexpr unsigned PARSED_DST_IDX[] = {1, 1, 4 /* + OpXSrcNum */};
+  static constexpr unsigned PARSED_SRC_IDX[] = {2, 2, 5 /* + OpXSrcNum */};
+
----------------
rename to FIRST_PARSED_SRC_IDX


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:595
+  unsigned getOperandsNum() const {
+    return Component::DST_NUM + SrcOperandsNum;
+  }
----------------
This method appears unused, and it is a bit confusingly named, contrasted with getNumOperands


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:611
+private:
+  bool hasMandatoryLiteral(unsigned SrcIdx) const {
+    assert(SrcIdx < Component::MAX_SRC_NUM);
----------------
Method name is confusing. Maybe it should be isIdxMandatoryLiteralOp


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:651
+  // Check VOPD operands constraints.
+  // GetRegIdx(Component, OperandIdx) must return a VGPR register index
+  // for the specified component and operand. The callback must return 0
----------------
It seems like GetRegIdx does not need Component as a parameter. For example in getVRegIdx the parameter is unnamed and unused. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134960/new/

https://reviews.llvm.org/D134960