[PATCH] D129767: [AMDGPU][MC][GFX11] AsmParser for op_sel for VOP3 dpp opcodes

Thu Jul 14 07:33:22 PDT 2022

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

Overall looks good. Some minor comments.

================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:8029

-void AMDGPUAsmParser::cvtVOP3OpSel(MCInst &Inst, const OperandVector &Operands) {
-  cvtVOP3P(Inst, Operands);
-
+void cvtVOP3DstOpSelOnly(MCInst &Inst) {
   int Opc = Inst.getOpcode();
----------------
This is related to the existing behavior of the function, not your change, but this function really needs a documentation comment. Something like "Determines which bit DST_OP_SEL occupies in the op_sel operand according to the number of src operands present, then copies that bit into src0_modifiers"

================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:8038
+       ++SrcNum)
+    ;
   assert(SrcNum > 0);
----------------
Weird semi-colon,  clang-format please

================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:8042
   int OpSelIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::op_sel);
+  if (OpSelIdx == -1)
+    return;
----------------
Move this check to the beginning of the function for early exit.

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

https://reviews.llvm.org/D129767