[PATCH] D137952: [AMDGPU][GFX11] Refactor VOPD operands handling

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash added a comment.

In D137952#3925471 <https://reviews.llvm.org/D137952#3925471>, @dp wrote:

>> Why does it change the error position?
>
> Before this change `getParsedSrcIndex` handled tied operands as if they were parsed and returned dst index for tied operands:
>
>   unsigned getParsedSrcIndex(unsigned SrcIdx, bool ComponentHasSrc2Acc) const {
>     if (ComponentHasSrc2Acc && SrcIdx == (MAX_SRC_NUM - 1))
>       return getParsedDstIndex();
>     ...
>   }
>
> This patch corrects handling of parsed operands: now they don't include tied ones.

Thanks for the explanation. Then this patch is nicely beneficial, but perhaps not NFC.



================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:8501
+    auto ParsedSrcOperandsNum = InstInfo[CompIdx].getParsedSrcOperandsNum();
+    for (unsigned SrcIdx = 0; SrcIdx < ParsedSrcOperandsNum; ++SrcIdx)
+      addOp(CInfo.getParsedSrcIndex(SrcIdx));
----------------
dp wrote:
> Joe_Nash wrote:
> > Can we rename SrcIdx to something like MCSrcIdx? So that there is no confusion with the ParsedSrcIndex returned by getParsedSrcIndex.
> I agree that there is some confusion with naming. It should be clear when the code operates with indices to opcode operands, indices to parsed operands and indices to MC operands.
> 
> But `MC` prefix would suggest iteration over MCInst operands, would it not? Maybe `OpSrcIdx` would be a better choice?
> 
Now I see that it does not iterate over MCInst operands. So clearly it is confusing :). 
SrcIdx is an index into parsed **src **operands which is a subset of parsed operands. 
So maybe these renames are most accurate:

getParsedSrcIndex -> getParsedOpIdxFromSrcIdx
SrcIdx -> CombinedSrcIdx
getParsedDstIndex -> getParsedOpIdxOfDst

In getRegIndicies
I think SrcIdx is acutally ComponentSrcIdx
Renames:
SrcIdx -> ComponentSrcIdx
getSrcIndex -> getCombinedMCSrcIdxFromComponentSrcIdx

obviously this is getting really verbose, but that's probably better than confusingly named. 




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

https://reviews.llvm.org/D137952