[PATCH] D92312: [IR] Disallow scalable vectors in ShuffleVectorInst::isExtractSubvectorMask

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/include/llvm/IR/Instructions.h:2281
+    // case.
+    if (isa<ScalableVectorType>(getType()))
+      return false;
----------------
c-rhodes wrote:
> david-arm wrote:
> > To be honest, it feels a bit inconsistent that in one variant of the function we assert it's a FixedVectorType, but we permit it in the other. Is it worth making both variants assert we're dealing with a FixedVectorType and changing the callsites to only call for FixedVectorTypes? It looks like we only these functions called from two places.
> > To be honest, it feels a bit inconsistent that in one variant of the function we assert it's a FixedVectorType, but we permit it in the other. Is it worth making both variants assert we're dealing with a FixedVectorType and changing the callsites to only call for FixedVectorTypes? It looks like we only these functions called from two places.
> 
> I initially implemented these identically to return false for scalable vectors, but then I realised for the other variant it's more of a stretch to incorrectly call it with a scalable vector since it's static and asks for a fixed number of elements. If we don't want the inconsistency I think my preference would be replacing the assert with what we have here
OK fair enough. I do think it would be good to be consistent in how we deal with scalable vectors for all of these shuffle vector functions. To be honest, I wonder if the code should even be calling this function for scalable vectors because it doesn't really make sense. I took a look at getUserCost where this function is called and there are lots of other functions we call, such as changesLength, isSelect, and so on. I imagine if we go down the path of allowing these functions to be called for scalable vectors, then all the other functions will need additional checks too. For example, see ./llvm/include/llvm/IR/Instructions.h:isTransposeMask.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92312