Rename interface functions and operands to make code clearer.
Details
Diff Detail
Unit Tests
Time | Test | |
---|---|---|
1,540 ms | x64 debian > libFuzzer.libFuzzer::fuzzer-finalstats.test |
Event Timeline
! 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 -> getParsedOpIdxOfDstIn getRegIndicies
I think SrcIdx is acutally ComponentSrcIdx
Renames:
SrcIdx -> ComponentSrcIdx
getSrcIndex -> getCombinedMCSrcIdxFromComponentSrcIdxobviously 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. |
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h | ||
---|---|---|
667 | Good point, thanks! |
Updated as suggested by Joe:
getDstIndexInMCOperands -> getIndexOfDstInMCOperands getSrcIndexInMCOperands -> getIndexOfSrcInMCOperands getDstIndexInParsedOperands -> getIndexOfDstInParsedOperands getSrcIndexInParsedOperands -> getIndexOfSrcInParsedOperands
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.