[SVE] Do not store a bool for Scalable in VectorType

[ctetreau]

Summary

• Whether or not a vector is scalable is a function of its type. Since

all instances of ScalableVectorType will have true for this value and
all instances of FixedVectorType will have false for this value, there
is no need to store it as a class member.

[fpetrogalli]

Hi @ctetreau ,

thank you for working on this!

Nit: the name ElementQuantity seems to refer to some exotic property of a VectorType, while all it refers to it is the minimum number of lanes of the vector that are available in the vector. To give a direct meaning to the name of the field, I think you should rename it MinNumberOfLanes, or KnownMinNumberOfLanes, and rename getElementQuantity accordingly. Getting the minimum number of lanes is an operation that makes perfectly sense for both Scalable and Fixed Type vectors, so there should be no controversies in defining this method in VectorType and let scalable and fixed vector types inherit it?

Thank you,

Francesco

llvm/include/llvm/IR/DerivedTypes.h

407

I think it would be great to explain the different meaning here, for both FixedVectorType and ScalableVectorType.

Something like:

/// The element quantity of this vector. The meaning of this value depends
/// the type of vector:
/// For FixedVectorType = <m x ty>, ElementQuantity = m
/// Fox ScalableVectorType = <vscale x m x ty>, ElementQuantity = m

[ctetreau]

Nit: the name ElementQuantity seems to refer to some exotic property of a VectorType, while all it refers to it is the minimum number of lanes of the vector that are available in the vector. To give a direct meaning to the name of the field, I think you should rename it MinNumberOfLanes, or KnownMinNumberOfLanes, and rename getElementQuantity accordingly. Getting the minimum number of lanes is an operation that makes perfectly sense for both Scalable and Fixed Type vectors, so there should be no controversies in defining this method in VectorType and let scalable and fixed vector types inherit it?

I specifically picked this name to keep its meaning as ambiguous as possible. Right now, for all existing vector types, it's a lower bound on the number of elements. Maybe at some point in the future, a new vector type is added where the quantity is not a lower bound. Perhaps this is just yak shaving though. Since the field is private, I don't mind changing it.

I prefer not to add a public function to base VectorType that gets just an unsigned right now. It's certainly a reasonable function, but my concern is that right now we're in a transition period, and most people will want to take the path of least resistance to fix their code. I think we'll see a lot of code that looks like this:

unsigned ActualNumElements = cast<VectorType>(Ty)->getNumElements();

... become:

unsigned ActualNumElements = cast<VectorType>(Ty)->getMinNumElements();

... instead of something more robust.

For now, I'll rename the field, and make it protected so that the derived vectors can directly return it instead of needing a getter.

[ctetreau]

address code review issues

[fpetrogalli]

Thank you for updating the patch @ctetreau

In D78601#1999638, @ctetreau wrote:

[...] Maybe at some point in the future, a new vector type is added where the quantity is not a lower bound.

If that's the case, I think that your name is better. Otherwise we will have to find a better name if someone comes up with a different use of the field for some fancy vector type that don't treat it as a "mi num of element". Apologies for the extra work!

I prefer not to add a public function to base VectorType that gets just an unsigned right now. It's certainly a reasonable function, but my concern is that right now we're in a transition period, and most people will want to take the path of least resistance to fix their code. I think we'll see a lot of code that looks like this:

unsigned ActualNumElements = cast<VectorType>(Ty)->getNumElements();

... become:

unsigned ActualNumElements = cast<VectorType>(Ty)->getMinNumElements();

... instead of something more robust.

Makes totally sense to me, thank you for explaining!

[fpetrogalli]

LGTM, feel free to rename the field and related method back to ElementQuantity`, as I said in my last comment.

Thank you,

Francesco

[ctetreau]

rename field back to ElementQuantity

[ctetreau]

I went ahead and renamed the field back to ElementQuantity. The comment should still be fine; if somebody adds a new vector type in the future, the comment won't be incorrect, just incomplete.

[fpetrogalli]

L very GTM, thanks!

[ctetreau]

No problem, thanks for doing code review!

llvm/include/llvm/IR/DerivedTypes.h

class VectorType : public Type {
   ///
   /// <4 x i32>          - a vector containing 4 i32s
   /// <vscale x 4 x i32> - a vector containing an unknown integer multiple
   ///                      of 4 i32s
 
   /// The element type of the vector.
   Type *ContainedType;
 
-  /// The element count of this vector
-  ElementCount EC;
-
 protected:
-  VectorType(Type *ElType, ElementCount EC, Type::TypeID TID);
+  /// The minimum number of elements of this vector. The meaning of this
+  /// value depends on the type of vector:
+  /// - For FixedVectorType = <m x ty>, KnownMinNumElements = m. There are
+  ///   exactly m elements in this vector.
+  /// - For ScalableVectorType = <vscale x m x ty>, KnownMinNumElements = m.
+  ///   There are vscale * m elements in this vector, where vscale is a
+  ///   runtime-constant integer greater than 0.
+  const unsigned KnownMinNumElements;
+
+  VectorType(Type *ElType, unsigned EQ, Type::TypeID TID);
 
 public:
   VectorType(const VectorType &) = delete;
   VectorType &operator=(const VectorType &) = delete;
 
   /// Get the number of elements in this vector. It does not make sense to call
   /// this function on a scalable vector, and this will be moved into
   /// FixedVectorType in a future commit
static VectorType *getDoubleElementsVectorType(VectorType *VTy) {
     return VectorType::get(VTy->getElementType(), EltCnt*2);
   }
 
   /// Return true if the specified type is valid as a element type.
   static bool isValidElementType(Type *ElemTy);
 
   /// Return an ElementCount instance to represent the (possibly scalable)
   /// number of elements in the vector.
-  ElementCount getElementCount() const { return EC; }
+  inline ElementCount getElementCount() const;
 
   /// Returns whether or not this is a scalable vector (meaning the total
   /// element count is a multiple of the minimum).
   bool isScalable() const { return EC.Scalable; }
 
   /// Methods for support type inquiry through isa, cast, and dyn_cast.
   static bool classof(const Type *T) {
     return T->getTypeID() == FixedVectorTyID ||
            T->getTypeID() == ScalableVectorTyID;
   }
 };
 
 bool Type::isVectorTy() const { return isa<VectorType>(this); }
 
 /// Class to represent fixed width SIMD vectors
 class FixedVectorType : public VectorType {
 protected:
   FixedVectorType(Type *ElTy, unsigned NumElts)
-      : VectorType(ElTy, {NumElts, false}, FixedVectorTyID) {}
+      : VectorType(ElTy, NumElts, FixedVectorTyID) {}
 
 public:
   static FixedVectorType *get(Type *ElementType, unsigned NumElts);
 
   static bool classof(const Type *T) {
     return T->getTypeID() == FixedVectorTyID;
   }
 };
 
 /// Class to represent scalable SIMD vectors
 class ScalableVectorType : public VectorType {
 protected:
   ScalableVectorType(Type *ElTy, unsigned MinNumElts)
-      : VectorType(ElTy, {MinNumElts, true}, ScalableVectorTyID) {}
+      : VectorType(ElTy, MinNumElts, ScalableVectorTyID) {}
 
 public:
   static ScalableVectorType *get(Type *ElementType, unsigned MinNumElts);
 
   /// Get the minimum number of elements in this vector. The actual number of
   /// elements in the vector is an integer multiple of this value.
-  uint64_t getMinNumElements() const { return getElementCount().Min; }
+  uint64_t getMinNumElements() const { return KnownMinNumElements; }
 
   static bool classof(const Type *T) {
     return T->getTypeID() == ScalableVectorTyID;
   }
 };
 
+inline ElementCount VectorType::getElementCount() const {
+  return ElementCount(KnownMinNumElements, isa<ScalableVectorType>(this));
+}
+
 /// Class to represent pointers.
 class PointerType : public Type {
   explicit PointerType(Type *ElType, unsigned AddrSpace);
 
   Type *PointeeTy;
 
 public:
   PointerType(const PointerType &) = delete;

llvm/lib/IR/Type.cpp

return !ElemTy->isVoidTy() && !ElemTy->isLabelTy() &&
          !ElemTy->isMetadataTy() && !ElemTy->isFunctionTy() &&
          !ElemTy->isTokenTy();
 }
 
 //===----------------------------------------------------------------------===//
 //                          VectorType Implementation
 //===----------------------------------------------------------------------===//
 
-VectorType::VectorType(Type *ElType, ElementCount EC, Type::TypeID TID)
-    : Type(ElType->getContext(), TID), ContainedType(ElType), EC(EC) {
+VectorType::VectorType(Type *ElType, unsigned KMNE, Type::TypeID TID)
+    : Type(ElType->getContext(), TID), ContainedType(ElType),
+      KnownMinNumElements(KMNE) {
   ContainedTys = &ContainedType;
   NumContainedTys = 1;
 }
 
 VectorType *VectorType::get(Type *ElementType, ElementCount EC) {
   if (EC.Scalable)
     return ScalableVectorType::get(ElementType, EC.Min);
   else