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

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:552
+VOPD::InstInfo getVOPDInstInfo(const MCInstrDesc &OpX, const MCInstrDesc &OpY) {
+  return {OpX, OpY};
+}
----------------
Joe_Nash wrote:
> 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.
I have no idea either, but I do not like repetition. Corrected.


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


================
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
----------------
Joe_Nash wrote:
> It seems like GetRegIdx does not need Component as a parameter. For example in getVRegIdx the parameter is unnamed and unused. 
The `Component` operand is used in `checkVOPDRegConstraints` - we have two different components there. See https://reviews.llvm.org/D135084.


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

https://reviews.llvm.org/D134960