[PATCH] D88409: [SVE] Make ElementCount and TypeSize use a new PolySize class

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:55
   /// At least one element.
-  bool isNonZero() const { return Min != 0; }
+  bool isNonZero() const { return MinVal != 0; }
   /// A return value of true indicates we know at compile time that the number
----------------
NIT: Implement in terms of isZero


================
Comment at: llvm/include/llvm/Support/TypeSize.h:153
+  PolySize coefficientNextPowerOf2() const {
+    return PolySize((T)llvm::NextPowerOf2(MinVal), IsScalable);
+  }
----------------
NIT: prefer static_cast


================
Comment at: llvm/include/llvm/Support/TypeSize.h:174
+
+  constexpr ElementCount(PolySize<unsigned> V) : PolySize(V) {}
+
----------------
Why provide this implicit conversion from base PolySize to ElementCount?

This could potentially lead to bugs, especially in the future when we introduce true a Polynomial type with exponents and/or multiple variables.

Is this neccesary to support cases like `ElementCount EC = PolySize::getScalable(4)`?


================
Comment at: llvm/include/llvm/Support/TypeSize.h:194
+public:
+  constexpr TypeSize(PolySize<uint64_t> V) : PolySize(V) {}
+
----------------
same as above, this implicit conversion could be dangerous


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

https://reviews.llvm.org/D88409