[CostModel]Replace FixedVectorType by VectorType in costgetIntrinsicInstrCost

[CarolineConcatto]

Summary

This patch replaces FixedVectorType by VectorType in getIntrinsicInstrCost
in BasicTTIImpl.h. It re-arranges the scalable type test earlier return
and add tests for scalable types.
This patch is the first patch of many to fix getIntrinsicInstrCost for scalable
vector.
Patches to fix the intrinsics masked gather and masked scatter, vector reduce
and fshl/fshr will be in upcoming patches.

Depends on D91532

[samparker]

I think it would be a good idea to test that the paths through getCmpSelInstrCost and getArithmeticInstrCost work too.

llvm/include/llvm/CodeGen/BasicTTIImpl.h

1331

This can be hoisted above the loop, which is makes it a bit more clear that we're not handled scalarization here.

1335

nit: no need for brackets and shouldn't this just be kept as FixedVectorType anyway?

[CarolineConcatto]

Thank you @samparker for your review. I agree with you that we should also test the code path of getCmpSelInstrCost and getArithmeticInstrCost, but at the moment these code paths were/are broken for scalable vector.
We are working to make these paths work for scalable vectors by replacing FixedVector with VectorType and using the ElementType class.
These will be addressed in upcoming patches. I don’t think that we should fix these code paths in this patch because the chain to fix it is long and will make the patch long and complex
What I can do is add tests with XFAIL for now, and remove in the future.
Is this a good solution for now?

[samparker]

I'd prefer if we didn't knowingly create broken paths here... does it currently just crash in the AArch64 backend or are there also failures in the generic parts? I'm thinking that we could just have a separate switch statement which checks for supported opcodes for scalable vectors and protects us from going down bad paths. A TODO comment could also be added to document why and what needs fixing up.

[ctetreau]

I'd prefer if we didn't knowingly create broken paths here

It is unfortunate, but this is all fallout from the VectorType refactor. All of this code was calling getNumElements() without checking isScalable(). The general approach for doing the VectorType refactor was "if it unconditionally calls getNumElements, make it unconditionally cast to FixedVectorType. If it was broken before, it will still be broken now, but at least we'll get an assertion failure instead of it just silently miscompiling"

Unfortunately, that means that a great deal of this TTI stuff is broken for scalable vectors, since it just wasn't being exercised.

Once D91174 is merged, I think it would be useful if somebody went through and updated the default TTI stuff to just return an invalid cost before any unconditional cast to FixedVectorType. At this point, we can get the stub of the comprehensive scalable vector TTI test, that initially just checks that most things return an invalid cost. (A few things do currently work)

[CarolineConcatto]

• fix nit
• remove failing tests

[sdesmalen]

In D92094#2422952, @ctetreau wrote:

I'd prefer if we didn't knowingly create broken paths here

It is unfortunate, but this is all fallout from the VectorType refactor. All of this code was calling getNumElements() without checking isScalable(). The general approach for doing the VectorType refactor was "if it unconditionally calls getNumElements, make it unconditionally cast to FixedVectorType. If it was broken before, it will still be broken now, but at least we'll get an assertion failure instead of it just silently miscompiling"

Unfortunately, that means that a great deal of this TTI stuff is broken for scalable vectors, since it just wasn't being exercised.

That's right, the paths are already broken. This patch merely tries to fix up a few code-paths that specifically don't require FixedVectorType. In this case, only the scalarization strictly requires FixedVectorType in this function. Other code-paths fail due to the called functions not yet working for scalable vectors, but those should be fixed in separate patches (e.g. one patch to address reductions in getTypeBasedIntrinsicInstrCost, one for gathers/scatters in getGatherScatterOpCost and one that fixes getArithmeticInstrCost for the codepath exercised by fshl/fshr).

Once D91174 is merged, I think it would be useful if somebody went through and updated the default TTI stuff to just return an invalid cost before any unconditional cast to FixedVectorType. At this point, we can get the stub of the comprehensive scalable vector TTI test, that initially just checks that most things return an invalid cost. (A few things do currently work)

Yes we should do that. From having seen how this work is layered, it will be quite a substantial task, as it requires changing the interfaces of the TTI functions and all its callers. We already started to change some of the 'root' nodes of the callgraph in e.g. D92178 (LoopVectorize), D92238 (SCEVExpander) and the SLPVectorizer, as those changes are NFC until any of the TTI functions returns an Invalid, and we can already have it do the sensible thing (i.e. fallback to "not vectorize"). The TTI methods themselves should either propagate the Invalid state (which hopefully comes for free), or as you suggested, return an Invalid cost. When the TTI functions start returning Invalid, we'll need to add additional tests for the Vectorizer and SLPVectorizer to guard they're indeed doing the right thing.

llvm/include/llvm/CodeGen/BasicTTIImpl.h

1331

+1

1335

+1

llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll

15

even though the cost doesn't directly matter much, it may be useful to just fill in the value it uses currently, so that when we improve the cost-model for scalable vectors, we can see the changes we make.

[sdesmalen]

llvm/include/llvm/CodeGen/BasicTTIImpl.h

1331

This should still use FixedVectorType

[CarolineConcatto]

-address review's comment

[CarolineConcatto]

llvm/include/llvm/CodeGen/BasicTTIImpl.h

1331

Thank you @sdesmalen. I've missed that.

llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll

15

Fair enough! I've added the values.

[ctetreau]

... Yes we should do that. From having seen how this work is layered, it will be quite a substantial task ...

Yeah, the TTI changes were one of the most painful parts of the VectorType refactor. From my experience, you're not going to be able to break that up into small patches. There's a billion headers, and they're all tightly coupled. But at some point somebody has to go and update them to return InstructionCost anyways, that seems like a good time to do it.

I am not looking forward to resolving that merge conflict in my downstream...

[ctetreau]

I see a bunch of new intrinsics being evaluated, but only tests for CTLZ and CCTZ. Do the others have coverage somewhere already?

[CarolineConcatto]

Hi @ctetreau.
I don't know if you are asking tests for Scalable Vector or FixedVectorType.
For Scalable Vector:
I added on the commit message which intrinsics will follow this patch because these intrinsics are broken for scalable vector.
So new tests with scalable vector will fail.
The only intrinsic that works now for scalable vector is CCTT and CTLZ.
For FixedVectorType:
I believe that only gather and scatter does not have tests for FixedVector, but all the others have tests.

[ctetreau]

In D92094#2431032, @CarolineConcatto wrote:

Hi @ctetreau.
I don't know if you are asking tests for Scalable Vector or FixedVectorType.

As far as I can tell, this patch is only a behavioral change for scalable vectors, so those are the only tests the I'm asking about.

For Scalable Vector:
I added on the commit message which intrinsics will follow this patch because these intrinsics are broken for scalable vector.
So new tests with scalable vector will fail.
The only intrinsic that works now for scalable vector is CCTT and CTLZ.

Prior to your change, this code bails before trying to evaluate the cost of any of the intrinsics in the switch. Now, the function will try to evaluate all of the intrinsics in the switch, but you're saying that of those, only CCTT and CTLZ actually work, and are being tested. I see this as a regression.

Please move the if (isa<ScalableVectorType>(RetTy)) return BaseT::getIntrinsicInstrCost(ICA, CostKind); bailing out logic into the beginning of each switch branch that does not work/is not currently tested. These can be removed on a case-by-case basis in the patches that fix these branches.

[samparker]

Please move the if (isa<ScalableVectorType>(RetTy)) return BaseT::getIntrinsicInstrCost(ICA, CostKind); bailing out logic into the beginning of each switch branch that does not work/is not currently tested. These can be removed on a case-by-case basis in the patches that fix these branches.

+1

[CarolineConcatto]

• add if statement for scalable vector for Intrinsics that not work with vscale

[sdesmalen]

llvm/include/llvm/CodeGen/BasicTTIImpl.h

1244

Can you rewrite this as:

if (isa<ScalableVectorType>(RetTy))
  return BaseT::getIntrinsicInstrCost(ICA, CostKind)

so that you don't need to indent all the other lines?

[CarolineConcatto]

• replace if statement by early return when facing scalable vector

[sdesmalen]

llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll

15

Sorry, one more comment, can you also check for the instruction that you're checking the cost for?

[CarolineConcatto]

• add llvm-ir instruction to the value in the cost model tests

[david-arm]

LGTM with the nits addressed!

llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll

16

nit: Can you fix the minor typo here - CHECK-NETX?

26

nit: Can you fix the minor typo here - CHECK-NETX?

[CarolineConcatto]

-fix the work NEXT in the test

llvm/include/llvm/CodeGen/BasicTTIImpl.h

unsigned getIntrinsicInstrCost(const IntrinsicCostAttributes &ICA,
     // Assume that target intrinsics are cheap.
     Intrinsic::ID IID = ICA.getID();
     if (Function::isTargetIntrinsic(IID))
       return TargetTransformInfo::TCC_Basic;
 
     if (ICA.isTypeBasedOnly())
       return getTypeBasedIntrinsicInstrCost(ICA, CostKind);
 
-    // TODO: Handle scalable vectors?
     Type *RetTy = ICA.getReturnType();
-    if (isa<ScalableVectorType>(RetTy))
-      return BaseT::getIntrinsicInstrCost(ICA, CostKind);
 
     ElementCount VF = ICA.getVectorFactor();
     ElementCount RetVF =
-        (RetTy->isVectorTy() ? cast<FixedVectorType>(RetTy)->getElementCount()
+        (RetTy->isVectorTy() ? cast<VectorType>(RetTy)->getElementCount()
                              : ElementCount::getFixed(1));
     assert((RetVF.isScalar() || VF.isScalar()) &&
            "VF > 1 and RetVF is a vector type");
     const IntrinsicInst *I = ICA.getInst();
     const SmallVectorImpl<const Value *> &Args = ICA.getArgs();
     FastMathFlags FMF = ICA.getFlags();
     switch (IID) {
     default:
unsigned getIntrinsicInstrCost(const IntrinsicCostAttributes &ICA,
 
     case Intrinsic::masked_scatter: {
       assert(VF.isScalar() && "Can't vectorize types here.");
       const Value *Mask = Args[3];
       bool VarMask = !isa<Constant>(Mask);
       Align Alignment = cast<ConstantInt>(Args[2])->getAlignValue();
       return thisT()->getGatherScatterOpCost(Instruction::Store,
                                              Args[0]->getType(), Args[1],
                                              VarMask, Alignment, CostKind, I);
     }
     case Intrinsic::masked_gather: {
       assert(VF.isScalar() && "Can't vectorize types here.");
       const Value *Mask = Args[2];
       bool VarMask = !isa<Constant>(Mask);
       Align Alignment = cast<ConstantInt>(Args[1])->getAlignValue();
       return thisT()->getGatherScatterOpCost(Instruction::Load, RetTy, Args[0],
                                              VarMask, Alignment, CostKind, I);
case Intrinsic::fshr: {
                                         CmpInst::BAD_ICMP_PREDICATE, CostKind);
         Cost +=
             thisT()->getCmpSelInstrCost(BinaryOperator::Select, RetTy, CondTy,
                                         CmpInst::BAD_ICMP_PREDICATE, CostKind);
       }
       return Cost;
     }
     }
+    // TODO: Handle the remaining intrinsic with scalable vector type
+    if (isa<ScalableVectorType>(RetTy))
+      return BaseT::getIntrinsicInstrCost(ICA, CostKind);
 
     // Assume that we need to scalarize this intrinsic.
     SmallVector<Type *, 4> Types;
     for (const Value *Op : Args) {
       Type *OpTy = Op->getType();
       assert(VF.isScalar() || !OpTy->isVectorTy());
       Types.push_back(VF.isScalar()
                           ? OpTy
                           : FixedVectorType::get(OpTy, VF.getKnownMinValue()));
     }
 
     if (VF.isVector() && !RetTy->isVoidTy())
       RetTy = FixedVectorType::get(RetTy, VF.getKnownMinValue());
 
     // Compute the scalarization overhead based on Args for a vector
     // intrinsic. A vectorizer will pass a scalar RetTy and VF > 1, while
     // CostModel will pass a vector RetTy and VF is 1.
     unsigned ScalarizationCost = std::numeric_limits<unsigned>::max();
     if (RetVF.isVector() || VF.isVector()) {
       ScalarizationCost = 0;
       if (!RetTy->isVoidTy())
         ScalarizationCost +=
             getScalarizationOverhead(cast<VectorType>(RetTy), true, false);
       ScalarizationCost +=
           getOperandsScalarizationOverhead(Args, VF.getKnownMinValue());

llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll

+; Checks getIntrinsicInstrCost in BasicTTIImpl.h with SVE for CTLZ and CCTZ
+
+; RUN: opt -cost-model -analyze -mtriple=aarch64--linux-gnu -mattr=+sve  < %s 2>%t | FileCheck %s
+
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
+; WARN-NOT: warning
+
+; Check for CTLZ
+
+define void  @ctlz_nxv4i32(<vscale x 4 x i32> %A) {
+; CHECK-LABEL: 'ctlz_nxv4i32'
+; CHECK-NEXT: Cost Model: Found an estimated cost of 1 for instruction:
+; CHECK-NETX: Cost Model: Found an estimated cost of 0  for instruction:
+  %1 = tail call <vscale x 4 x i32> @llvm.ctlz.nxv4i32(<vscale x 4 x i32> %A, i1 true)
+  ret void
+}
+
+; Check for CCTZ
+
+define void  @cttz_nxv4i32(<vscale x 4 x i32> %A) {
+; CHECK-LABEL: 'cttz_nxv4i32'
+; CHECK-NEXT: Cost Model: Found an estimated cost of 1 for instruction:
+; CHECK-NETX: Cost Model: Found an estimated cost of 0 for instruction:
+  %1 = tail call <vscale x 4 x i32> @llvm.cttz.nxv4i32(<vscale x 4 x i32> %A, i1 true)
+  ret void
+}
+
+declare <vscale x 4 x i32> @llvm.ctlz.nxv4i32(<vscale x 4 x i32>, i1)
+declare <vscale x 4 x i32> @llvm.cttz.nxv4i32(<vscale x 4 x i32>, i1)