[IR] Disallow scalable vectors in ShuffleVectorInst::isExtractSubvectorMask

[c-rhodes]

Summary

It's not possible to express an extract subvector shuffle mask for
a scalable vector.

[david-arm]

llvm/include/llvm/IR/Instructions.h

2271

Is it worth updating the text in the assert, i.e. "Shuffle needs fixed width vector constant"?

2281

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.

[c-rhodes]

llvm/include/llvm/IR/Instructions.h

2281

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

[david-arm]

llvm/include/llvm/IR/Instructions.h

2281

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.

[c-rhodes]

llvm/include/llvm/IR/Instructions.h

2281

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.

[c-rhodes]

Replace assert with bail out for scalable vector in static variant.

[david-arm]

LGTM, thanks!

llvm/include/llvm/IR/Instructions.h

public:
 
   /// Return true if this shuffle mask is an extract subvector mask.
   /// A valid extract subvector mask returns a smaller vector from a single
   /// source operand. The base extraction index is returned as well.
   static bool isExtractSubvectorMask(ArrayRef<int> Mask, int NumSrcElts,
                                      int &Index);
   static bool isExtractSubvectorMask(const Constant *Mask, int NumSrcElts,
                                      int &Index) {
-    assert(Mask->getType()->isVectorTy() && "Shuffle needs vector constant.");
+    assert(isa<FixedVectorType>(Mask->getType()) &&
+           "Shuffle needs vector constant.");
     SmallVector<int, 16> MaskAsInts;
     getShuffleMask(Mask, MaskAsInts);
     return isExtractSubvectorMask(MaskAsInts, NumSrcElts, Index);
   }
 
   /// Return true if this shuffle mask is an extract subvector mask.
   bool isExtractSubvectorMask(int &Index) const {
+    // Not possible to express a shuffle mask for a scalable vector for this
+    // case.
+    if (isa<ScalableVectorType>(getType()))
+      return false;
+
     int NumSrcElts =
         cast<FixedVectorType>(Op<0>()->getType())->getNumElements();
     return isExtractSubvectorMask(ShuffleMask, NumSrcElts, Index);
   }
 
   /// Change values in a shuffle permute mask assuming the two vector operands
   /// of length InVecNumElts have swapped position.
   static void commuteShuffleMask(MutableArrayRef<int> Mask,

llvm/unittests/IR/InstructionsTest.cpp

TEST(InstructionsTest, ShuffleMaskQueries) {
   // TODO: IdentityWithPadding should be true here even though the high mask values are not undef.
   ShuffleVectorInst *Id12 = new ShuffleVectorInst(V0, ConstantVector::get({CU, CU, CU, CU}),
                                                   ConstantVector::get({C0, CU, C2, C3, CU, CU, C6, C7}));
   EXPECT_FALSE(Id12->isIdentity());
   EXPECT_FALSE(Id12->isIdentityWithPadding());
   EXPECT_FALSE(Id12->isIdentityWithExtract());
   EXPECT_FALSE(Id12->isConcat());
   delete Id12;
+
+  // Not possible to express shuffle mask for scalable vector for extract
+  // subvector.
+  Type *VScaleV4Int32Ty = ScalableVectorType::get(Int32Ty, 4);
+  ShuffleVectorInst *Id13 =
+      new ShuffleVectorInst(Constant::getAllOnesValue(VScaleV4Int32Ty),
+                            UndefValue::get(VScaleV4Int32Ty),
+                            Constant::getNullValue(VScaleV4Int32Ty));
+  int Index = 0;
+  EXPECT_FALSE(Id13->isExtractSubvectorMask(Index));
+  delete Id13;
 }
 
 TEST(InstructionsTest, GetSplat) {
   // Create the elements for various constant vectors.
   LLVMContext Ctx;
   Type *Int32Ty = Type::getInt32Ty(Ctx);
   Constant *CU = UndefValue::get(Int32Ty);
   Constant *C0 = ConstantInt::get(Int32Ty, 0);