[PATCH] D75478: [SVE] Make getNumElements assert if the vector is scalable

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau created this revision.
Herald added subscribers: llvm-commits, psnobl, rkruppe, hiraditya, tschuett.
Herald added a reviewer: efriedma.
Herald added a project: LLVM.
ctetreau updated this revision to Diff 247722.
ctetreau added a comment.

fix commit message formatting


  VectorType inherits getNumElements from SequentialType. For a fixed

length vector, this returns the number of vector lanes. Previously, for
scalable vectors, this has returned the n for <vscale x n x ty>.
VectorType has another function getElementCount that returns a pair of
(bool, uint64_t) where the bool is true if the vector is scalable, and
min is the same as the return value of getNumElements. This form is
preferred because it provides the caller a way of knowing if the vector
is scalable.

  There exists a large amount of code that calls getNumElements and

iterates over the returned value. This code is correct for fixed length
vectors, but is always a bug for scalable vectors since the number of
vector lanes in a scalable vector is unknown at compile time. To
mitigate bugs that are caused by this situation, we are changing the
behavior of getNumElements to assert that if the SequentialType instance
is a vector, that the vector is not scalable. This should be a minimally
intrusive change since very little code has been written that uses
getNumElements correctly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75478

Files:
  llvm/include/llvm/IR/DerivedTypes.h
  llvm/lib/IR/Type.cpp


Index: llvm/lib/IR/Type.cpp
===================================================================
--- llvm/lib/IR/Type.cpp
+++ llvm/lib/IR/Type.cpp
@@ -577,6 +577,21 @@
   return true;
 }
 
+//===----------------------------------------------------------------------===//
+//                       SequentialType Implementation
+//===----------------------------------------------------------------------===//
+
+uint64_t SequentialType::getNumElements() const {
+#ifndef NDEBUG
+  if (auto *VTy = dyn_cast<VectorType>(this)) {
+    assert(!VTy->isScalable() &&
+           "The length of a scalable vector is unknown at compile time");
+  }
+#endif
+
+  return NumElements;
+}
+
 //===----------------------------------------------------------------------===//
 //                           ArrayType Implementation
 //===----------------------------------------------------------------------===//
@@ -609,7 +624,7 @@
 //===----------------------------------------------------------------------===//
 
 VectorType::VectorType(Type *ElType, ElementCount EC)
-  : SequentialType(VectorTyID, ElType, EC.Min), Scalable(EC.Scalable) {}
+  : SequentialType(VectorTyID, ElType, EC.Min), EC(EC) {}
 
 VectorType *VectorType::get(Type *ElementType, ElementCount EC) {
   assert(EC.Min > 0 && "#Elements of a VectorType must be greater than 0");
Index: llvm/include/llvm/IR/DerivedTypes.h
===================================================================
--- llvm/include/llvm/IR/DerivedTypes.h
+++ llvm/include/llvm/IR/DerivedTypes.h
@@ -391,9 +391,8 @@
   SequentialType(const SequentialType &) = delete;
   SequentialType &operator=(const SequentialType &) = delete;
 
-  /// For scalable vectors, this will return the minimum number of elements
-  /// in the vector.
-  uint64_t getNumElements() const { return NumElements; }
+  /// For scalable vectors this value is undefined
+  uint64_t getNumElements() const;
   Type *getElementType() const { return ContainedType; }
 
   /// Methods for support type inquiry through isa, cast, and dyn_cast.
@@ -441,13 +440,12 @@
   /// <vscale x 4 x i32> - a vector containing an unknown integer multiple
   ///                      of 4 i32s
 
-  VectorType(Type *ElType, unsigned NumEl, bool Scalable = false);
   VectorType(Type *ElType, ElementCount EC);
 
-  // If true, the total number of elements is an unknown multiple of the
-  // minimum 'NumElements' from SequentialType. Otherwise the total number
-  // of elements is exactly equal to 'NumElements'.
-  bool Scalable;
+  // If EC.Scalable is true, the total number of elements is an unknown multiple
+  // of the EC.Min. Otherwise the total number of elements is exactly equal to
+  // SequentialType::getNumElements().
+  ElementCount EC;
 
 public:
   VectorType(const VectorType &) = delete;
@@ -538,15 +536,21 @@
   /// Return an ElementCount instance to represent the (possibly scalable)
   /// number of elements in the vector.
   ElementCount getElementCount() const {
-    uint64_t MinimumEltCnt = getNumElements();
-    assert(MinimumEltCnt <= UINT_MAX && "Too many elements in vector");
-    return { (unsigned)MinimumEltCnt, Scalable };
+#ifndef NDEBUG
+    if (!EC.Scalable) {
+      uint64_t MinimumEltCnt = getNumElements();
+      assert(MinimumEltCnt <= UINT_MAX && "Too many elements in vector");
+      assert(MinimumEltCnt == EC.Min &&
+             "Mismatch between stored element count and getNumElements()");
+    }
+#endif
+    return EC;
   }
 
   /// Returns whether or not this is a scalable vector (meaning the total
   /// element count is a multiple of the minimum).
   bool isScalable() const {
-    return Scalable;
+    return EC.Scalable;
   }
 
   /// Return the minimum number of bits in the Vector type.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D75478.247722.patch
Type: text/x-patch
Size: 3740 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200302/bbdccc6c/attachment.bin>