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

Wed May 6 18:12:28 PDT 2020

efriedma 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);
----------------
ctetreau wrote:
> 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?
At some point, we should fix IITDescriptor to do something sane, yes.  It will be easier to refactor if the existing code doesn't have known bugs.

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

https://reviews.llvm.org/D79416