[PATCH] D93043: [CostModel] Add costs for llvm.experimental.vector.{extract,insert} intrinsics

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:121
            "Can only extract subvectors from vectors");
     int NumSubElts = SubVTy->getNumElements();
+    assert((!isa<FixedVectorType>(VTy) ||
----------------
bsmith wrote:
> CarolineConcatto wrote:
> > If this starts to accept scalable vector, by replacing FixedVectorType to VectorType, this could be a problem. 
> > See this function unsigned VectorType::getNumElements() const; inDerivedTypes.h
> > Maybe you will need to use InstructionCost class or just assert at the moment when the type is scalable
> Is adding another assert is really necessary? In my mind if someone changed SubVTy from FixedVectorType to VectorType they should also make this function support scalable vectors.
> 
> The fact that SubVTy is a FixedVectorType means there already is an assert and statement that this function doesn't support scalable vectors for SubVTy.
The problem here is also that you are using NumSubElts to compute the cost.
For scalable vector the number of elements is not know at compiler time, only when in run time.
So, this is cost is not entirely correct, because it computes only the minimum cost.
I don't know if you looked the function VectorType::getNumElements() , but it return getKnonwMinValue().
For this example  <vscale x 16 x i32> it would be 16, but you still needs to know the value of vscale.
Because the total number of elements if vscale * 16, for this example.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93043/new/

https://reviews.llvm.org/D93043