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

[Joe Ellis via Phabricator via llvm-commits]

joechrisellis added a comment.

Hi @bsmith, thank you for the patch! 😄

I noticed `b3e77c6d55853eea5f5c32ec8a3510c0b0e438e1` earlier:

  commit b3e77c6d55853eea5f5c32ec8a3510c0b0e438e1
  Author: Christopher Tetreault <ctetreau at quicinc.com>
  Date:   Tue Jun 16 14:05:21 2020 -0700
  
      [SVE] Remove invalid calls to VectorType::getNumElements from BasicTTIImpl
  
      Summary:
      Most of these operations are reasonable for scalable vectors. Due to
      this, we have decided not to change the interface to specifically take
      FixedVectorType despite the fact that the current implementations make
      fixed width assumptions. Instead, we cast to FixedVectorType and assert
      in the body. If a developer makes some change in the future that causes
      one of these asserts to fire, they should either change their code or
      make the function they are trying to call handle scalable vectors.
  
      Reviewers: efriedma, samparker, RKSimon, craig.topper, sdesmalen, c-rhodes
  
      Reviewed By: efriedma
  
      Subscribers: tschuett, rkruppe, psnobl, llvm-commits
  
      Tags: #llvm
  
      Differential Revision: https://reviews.llvm.org/D81495

This patch changes the interface of both `get{Extract,Insert}SubvectorOverhead` (among others) to accepting `FixedVectorType`s instead of `VectorType`s, so this patch is partially reverting some of the changes there. Not sure if this is pertinent here, but perhaps it's useful for context.



================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:118
+  unsigned getExtractSubvectorOverhead(VectorType *VTy, int Index,
                                        FixedVectorType *SubVTy) {
     assert(VTy && SubVTy &&
----------------
`llvm.experimental.vector.extract` can also extract a scalable from a scalable -- should we also change this `FixedVectorType` to `VectorType` too?

I suppose this cost model breaks down a bit when we're extracting/inserting a scalable into a scalable, since we don't know how many elements we have to 'work on'. But I think that it might be possible for this code to crash in the same way as before if we don't address this case.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:143
+  unsigned getInsertSubvectorOverhead(VectorType *VTy, int Index,
                                       FixedVectorType *SubVTy) {
     assert(VTy && SubVTy &&
----------------
Same as above -- do we want to change this too?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-vec-insert-extract.ll:21
+
+declare <16 x i32> @llvm.experimental.vector.extract.v16i32.nxv4i32(<vscale x 4 x i32>, i64)
+declare <vscale x 4 x i32> @llvm.experimental.vector.insert.nxv4i32.v16i32(<vscale x 4 x i32>, <16 x i32>, i64)
----------------
Related to the above comments, could we add some tests for inserting/extracting a scalable from a scalable? 😄 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93043