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

[Ivan Kosarev via Phabricator via llvm-commits]

kosarev added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:6036
+      else
+        break;
+    } else if (trySkipId("glc"))
----------------
dp wrote:
> I think it is not clear enough that the purpose of this `break` it to distinguish first and subsequent iterations.
I think the intention was to just make it stop on non-policy tokens.


================
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]);
----------------
dp wrote:
> 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.
Good points. This clearly deserves a rewrite.


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:9110
 
+OperandMatchResultTy
+AMDGPUAsmParser::parseCustomOperand(OperandVector &Operands, unsigned MCK) {
----------------
dp wrote:
> I think this code is less readable and maintainable than the `AMDGPUOptionalOperandTable`.
The new code is a bit repetitive, but there are ways to deal with that. (I'm going to.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138492