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

[Bradley Smith via Phabricator via llvm-commits]

bsmith 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) ||
----------------
CarolineConcatto wrote:
> CarolineConcatto wrote:
> > 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.
> But after saying all that I  think this part is OK
> Because you are still keeping FixedVectorType *SubVTy.
Correct, if this function started taking a scalable vector for SubVTy, the use of getNumElements would need to change. For now in that case it falls back to `BaseT::getIntrinsicInstrCost(ICA, CostKind)` before getting to this function, hence should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93043