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

Cullen Rhodes via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 1 07:06:48 PST 2020


c-rhodes added inline comments.


================
Comment at: llvm/include/llvm/IR/Instructions.h:2281
+    // case.
+    if (isa<ScalableVectorType>(getType()))
+      return false;
----------------
david-arm wrote:
> 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.
I think either way any functions where scalable vectors don't make sense will require some form of check, I think with what you're proposing by changing the callers to only be entered for fixed vectors it implies adding asserts the underlying vector type isn't scalable? Otherwise similar calls could be introduced in the future. It depends on the function of course, for the non-static variant here it crashes today for scalable vectors since it explicitly uses `FixedVectorType`, but that's not true across all functions. We already bail out for scalable vectors in `isIdentityWithPadding` and `isIdentityWithExtract`. I think `changesLength` already support scalable vectors since the length of the llvm::SmallVector shufflemask relates to the minimum number of elements, I'm going to update `increasesLength` similarly. I agree there are other functions in the interface that need considering for scalable vectors, I think the ones using `FixedVectorType` are higher priority since they crash today. I'd rather not change the callers, I think it adds complication. I'll replace the assert in the static variant to bail out as we do here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92312



More information about the llvm-commits mailing list