[AMDGPU][GFX11] Refactor VOPD operands handling

[dp]

Summary

Correct VOPD ComponentLayout to explicitly use ComponentProps of previous component (if any). Hopefully this will make code more clear.

This is actually an NFC change. It only affects one existing test for error position.

[Joe_Nash]

This looks like a reasonable refactor. Why does it change the error position?

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

8501

Can we rename SrcIdx to something like MCSrcIdx? So that there is no confusion with the ParsedSrcIndex returned by getParsedSrcIndex.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

532

I think there should be a documentation comment for this function.

[dp]

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.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

8501

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?

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

545

Another problem with naming is here: getSrcIndex(SrcIdx) looks awful to me. How about renaming getSrc* (and other similar functions) to getMCSrc*?

[Joe_Nash]

In 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.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

8501

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.

[dp]

Add a comment describing InstInfo::getRegIndices.

[dp]

I suggest doing renaming in a separate patch because it is a separate problem introduced before this change.

[dp]

Rebase.

[Joe_Nash]

In D137952#3927187, @dp wrote:

I suggest doing renaming in a separate patch because it is a separate problem introduced before this change.

Ok.

This patch LGTM