[PATCH] D138492: [AMDGPU][AsmParser] Refine parsing instruction operands.

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

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

LGTM. Nice improvements!



================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:6036
+      else
+        break;
+    } else if (trySkipId("glc"))
----------------
I think it is not clear enough that the purpose of this `break` it to distinguish first and subsequent iterations.


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:6075
+    AMDGPUOperand *CPolOp = nullptr;
+    for (unsigned I = 1; I != Operands.size(); ++I) {
+      AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[I]);
----------------
Is it possible to avoid this search on each iteration? Ideally, the code should accumulate `cpol` values in the loop, but store the result after exiting the loop. 

Also, I'm not sure if it is possible to have 2 `cpol` operands separated by other operands. Maybe the search of a preceding `cpol` operand is not needed.


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:9110
 
+OperandMatchResultTy
+AMDGPUAsmParser::parseCustomOperand(OperandVector &Operands, unsigned MCK) {
----------------
I think this code is less readable and maintainable than the `AMDGPUOptionalOperandTable`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138492