[PATCH] D79416: [SVE] Fix wrong usage of getNumElements() in matchIntrinsicType

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/lib/IR/Function.cpp:1184
+      // TODO: It would be nice if we could assert that we'd seen
+      // a ScalableVecArgument prior to this for ScalableVectorTypes
       VectorType *VT = dyn_cast<VectorType>(Ty);
----------------
efriedma wrote:
> I don't think we should allow a ScalableVectorType here.  Instead, I think we should fix the recursive call to matchIntrinsicType for IITDescriptor::ScalableVecArgument to pass down an appropriate FixedVectorType, and then `dyn_cast<FixedVectorType>(Ty)` here.
> 
> Yes, that's a little weird, but better to make the weirdness self-consistent; my suggestion is consistent with the way we handle ScalableVecArgument in other places.
Is this a temporary measure until IITDescriptor is fixed? I think "little weird" is a bit of an understatement.

Would it not be better to just do getElementCount().Min here, and make it official that it "works" for scalable vectors and fixed vectors?


================
Comment at: llvm/lib/IR/Function.cpp:1361
+      ScalableVectorType *STy = cast<ScalableVectorType>(Ty);
+      unsigned MinElts = STy->getElementCount().Min;
+      FixedVectorType *FVTy =
----------------
NIT: ScalableVectorType actually has a getMinNumElements(). Really, getElementCount() is only really useful if you are dealing with base VectorType objects.


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

https://reviews.llvm.org/D79416