This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][GFX11] Refactor VOPD operands handling
ClosedPublic

Authored by dp on Nov 14 2022, 7:30 AM.

Details

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.

Diff Detail

Event Timeline

dp created this revision.Nov 14 2022, 7:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 7:30 AM
dp requested review of this revision.Nov 14 2022, 7:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 7:30 AM

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

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

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 added a comment.Nov 14 2022, 10:41 AM

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
8528

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*?

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
8528

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 updated this revision to Diff 475407.Nov 15 2022, 3:06 AM
dp marked an inline comment as done.

Add a comment describing InstInfo::getRegIndices.

dp added a comment.Nov 15 2022, 3:09 AM

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

dp updated this revision to Diff 475431.Nov 15 2022, 5:00 AM

Rebase.

Joe_Nash accepted this revision.Nov 15 2022, 6:39 AM
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

This revision is now accepted and ready to land.Nov 15 2022, 6:39 AM
This revision was landed with ongoing or failed builds.Nov 16 2022, 5:29 AM
This revision was automatically updated to reflect the committed changes.