This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][GFX11][NFC] Refactor VOPD operands handling (part 2)
ClosedPublic

Authored by dp on Nov 16 2022, 6:51 AM.

Details

Summary

Rename interface functions and operands to make code clearer.

Diff Detail

Event Timeline

dp created this revision.Nov 16 2022, 6:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 6:51 AM
dp requested review of this revision.Nov 16 2022, 6:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 6:51 AM
dp added a comment.Nov 16 2022, 6:53 AM

! In D137952, @Joe_Nash wrote:
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.

I have not exactly followed your rename suggestions, but I'll try to explain the rationale.

The interface provided by ComponentLayout, ComponentProps and ComponentInfo always
takes component operand indices for input (i.e. Component::DST, Component::SRC0, etc.)
The interface provides functions to map these component indices to either MachineInstr/MCInst
operand indices or parsed operand indices. So I think the suffix "FromComponentSrcIdx" is not necessary.
Also note that there is only one MachineInstr/MCInst/parsed operands array for each component
so there is no ambiguity (no need to specify the "combined" suffix).

I renamed some functions and their operands to make the mapping clear, so now we have the following interface:

unsigned ComponentLayout::getDstIndexInMCOperands();
unsigned ComponentLayout::getSrcIndexInMCOperands(unsigned CompSrcIdx);

unsigned ComponentLayout::getDstIndexInParsedOperands();
unsigned ComponentLayout::getSrcIndexInParsedOperands(unsigned CompSrcIdx);
 
unsigned ComponentProps::getCompSrcOperandsNum();
unsigned ComponentProps::getCompParsedSrcOperandsNum();

The complete list of renames is below:

getSrcOperandsNum        -> getCompSrcOperandsNum
getParsedSrcOperandsNum  -> getCompParsedSrcOperandsNum
getMandatoryLiteralIndex -> getMandatoryLiteralCompOperandIndex
getDstIndex              -> getDstIndexInMCOperands
getSrcIndex              -> getSrcIndexInMCOperands
getParsedDstIndex        -> getDstIndexInParsedOperands
getParsedSrcIndex        -> getSrcIndexInParsedOperands
getParsedOperandIndex    -> getIndexInParsedOperands
getInvalidOperandIndex   -> getInvalidCompOperandIndex

See my comment, otherwise I like it.

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
667

Does (Index of Src) vs. (Src Index) imply something different to you? To me it does. My instinct is that (Index of Src) and (Index of Dst) sample from the same list, whereas (Src Index) and (Dst Index) sample from different lists.

So to me, I would prefer the names getIndexOfDstInParsedOperands and getIndexOfSrcInParsedOperands, since these sample from the same list, the list of parsed operands.

dp marked an inline comment as done.Nov 17 2022, 3:24 AM
dp added inline comments.
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
667

Good point, thanks!

dp updated this revision to Diff 476075.Nov 17 2022, 3:25 AM
dp marked an inline comment as done.

Updated as suggested by Joe:

getDstIndexInMCOperands      -> getIndexOfDstInMCOperands     
getSrcIndexInMCOperands      -> getIndexOfSrcInMCOperands     
getDstIndexInParsedOperands  -> getIndexOfDstInParsedOperands 
getSrcIndexInParsedOperands  -> getIndexOfSrcInParsedOperands
This revision is now accepted and ready to land.Nov 17 2022, 6:29 AM
This revision was landed with ongoing or failed builds.Nov 18 2022, 3:15 AM
This revision was automatically updated to reflect the committed changes.