[CostModel] TargetTransformInfoImplCRTPBase.getInstructionCost - move shuffle analysis into improveShuffleKindFromMask / target getShuffleCost

[RKSimon]

As discussed on #144690 and #141634 - we're making too many assumptions about how length changing shuffles are legalized by the target. These need to be driven by the actual target implementations.

Additionally - like getInstructionCost - improveShuffleKindFromMask needs to be able to recognize a SK_PermuteSingleSrc class shuffle that has been miss-tagged as a SK_PermuteTwoSrc class.

CC @davemgreen @preames

llvm-project/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

Lines 1527 to 1547 in bb8c42e

	case Instruction::ShuffleVector: {
	auto *Shuffle = dyn_cast<ShuffleVectorInst>(U);
	if (!Shuffle)
	return TTI::TCC_Basic; // FIXME
	auto *VecTy = cast<VectorType>(U->getType());
	auto *VecSrcTy = cast<VectorType>(Operands[0]->getType());
	ArrayRef<int> Mask = Shuffle->getShuffleMask();
	int NumSubElts, SubIndex;
	// TODO: move more of this inside improveShuffleKindFromMask.
	if (Shuffle->changesLength()) {
	// Treat a 'subvector widening' as a free shuffle.
	if (Shuffle->increasesLength() && Shuffle->isIdentityWithPadding())
	return 0;
	if (Shuffle->isExtractSubvectorMask(SubIndex))
	return TargetTTI->getShuffleCost(TTI::SK_ExtractSubvector, VecTy,
	VecSrcTy, Mask, CostKind, SubIndex,
	VecTy, Operands, Shuffle);

[davemgreen]

I was looking into things over the weekend. My ideal would be that a call to getInstructionCost(Shuffle) and a call to getShuffleInstrCost(SK_PermuteTwoSrc, Shuffle->getType(), Shuffle->getOperandType(), ...) should produce the same result. Otherwise it makes it very difficult to test when they can produce different costs.

I attempted a couple of things, either to move the Shuffle->changesLength() logic into the backends that use it (but that led to a lot of mess, although some parts might be useful for AArch64). Another was to that move all of the logic in getInstructionCost into BasicTTIImpl::getShuffleCost so that getInstructionCost was a very simple wrapper to it. The existing getShuffleCost implementations were moved to getShuffleCostImpl so that a backend could override the base getShuffleCost if desired. It led to more test changes than I was hoping for though (see #145373), and I'm not sure it is a useful approach.

I was wondering if in the short term, if we needed it, we could have a function that asks the backend whether to call getShuffleCost directly without any of the adjustment in getInstructionCost, so that backends could slowly migrate over to it.

[pawan-nirpal-031]

Hi @davemgreen I think this is the exact problem I'm facing with my #165375 the costs returned by getShuffleCost are different, when I request the cost via cost-model, vs when I check via TTI.getShuffleCost via vector-combine. Should it be the case that the cost modelling code inside the following function should be merged with getShuffleCost as one source of shuffle costs? And such costs should anyway be inside Target TTI only and having them in TTIImpl doesn't look like a sound design.

getInstructionCost into BasicTTIImpl::getShuffleCost so that getInstructionCost was a very simple wrapper to it. The existing getShuffleCost implementations were moved to getShuffleCostImpl so that a backend could override the base getShuffleCost if desired. It led to more test changes than I was hoping for though (see #145373), and I'm not sure it is a useful approach.

This should be doable, are the test changes for all the target's too overwhelming to do it in one go?