[PATCH] D32530: [SVE][IR] Scalable Vector IR Type

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Seems fine in general, just some nits.



================
Comment at: include/llvm/IR/DerivedTypes.h:413
+  // minimum 'NumElements' from SequentialType. Otherwise the total number
+  // of elements is exactly equal to 'NumElements'
+  bool Scalable;
----------------
Nit: Punctuation (comments should end with .)


================
Comment at: include/llvm/IR/DerivedTypes.h:477
+  /// Return an ElementCount instance to represent the (possibly scalable)
+  /// number of elements in the vector
+  ElementCount getElementCount() const {
----------------
Nit: Punctuation.


================
Comment at: include/llvm/IR/DerivedTypes.h:490
+
+  /// Return the minimum number of bits in the Vector type.
   /// Returns zero when the vector is a vector of pointers.
----------------
Nit: I'd like to see a similar comment in SequentialType::getNumElements.


================
Comment at: include/llvm/Support/ScalableSize.h:26
+  bool Scalable; // if true, NumElements is a multiple of 'Min' determined
+                 // at runtime rather than compile time
+
----------------
Nit: Punctuation and capitalization (If [...])


================
Comment at: lib/IR/Verifier.cpp:311
   void checkAtomicMemAccessSize(Type *Ty, const Instruction *I);
+  static bool hasScalableVectorValue(const Type *Ty);
 
----------------
Nitpick: I would call this "containsScalableVectorValue", to make it clear that it doesn't just look at the top level type.


================
Comment at: lib/IR/Verifier.cpp:656
+    for (Type *EltTy : STy->elements())
+      Result |= hasScalableVectorValueRecursive(EltTy, Visited);
+
----------------
Could do an early return here instead of aggregating the result.


================
Comment at: lib/IR/Verifier.cpp:4797
+
+  // Ensure there are no scalable types in aggregates
+  V.verifyTypes(M);
----------------
Nitpick 1: This comment is going to become stale as soon as someone comes up with a non-scalable type they'd like to check.
Nitpick 2: Any reason why this is called here and not in Verifier::verify?


================
Comment at: unittests/IR/VectorTypesTest.cpp:34
+  VectorType *V2Int64Ty = VectorType::get(Int64Ty, EltCnt/2);
+  ASSERT_FALSE(V2Int64Ty->isScalable());
+  VectorType *V8Int64Ty = VectorType::get(Int64Ty, EltCnt*2);
----------------
I'd also check the number of elements and element size here, just to cover the ElementCount operators fully.


================
Comment at: unittests/IR/VectorTypesTest.cpp:69
+  ASSERT_TRUE(ScV4Int64Ty->isScalable());
+  VectorType *ScV2Int64Ty = VectorType::get(Int64Ty, EltCnt/2);
+  ASSERT_TRUE(ScV2Int64Ty->isScalable());
----------------
Ditto.


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

https://reviews.llvm.org/D32530