[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:
> 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.
It seems to me that it "works" with scalable vectors due to the number of hoops we're jumping through to ensure it actually gets called in the recursive call from ScalableVecArgument. I'd say that doing the explicit conversion from scalable to fixed vector is planting the seed for a bug. If this all needs to get reworked anyways, I think it would be better if this switch arm got changed rather than converting to fixed width vector.

Either way, whichever spot gets changed needs a FIXME comment


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

https://reviews.llvm.org/D79416