[PATCH] D88982: [NFCI] Add PolyBase as base class for ElementCount, TypeSize and StackOffset.

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:39
+/// for only a single dimension when true, or multiple coefficients when false.
+template <typename T, unsigned Dims, bool DimIsExclusive> class PolyBase {
 protected:
----------------
NIT: Nomenclature: suggest changing to IsUnivariate to match the terminology of the mathematical concept


================
Comment at: llvm/include/llvm/Support/TypeSize.h:41
 protected:
-  T MinVal;        // The minimum value that it could be.
-  bool IsScalable; // If true, the total value is determined by multiplying
-                   // 'MinVal' by a runtime determinded quantity, 'vscale'.
+  T Coefficients[Dims];
+  unsigned ExclusiveDim;
----------------
NIT: might as well use std::array


================
Comment at: llvm/include/llvm/Support/TypeSize.h:47
+  PolyBase(ArrayRef<typename std::enable_if<!D, T>::type> Values)
+      : ExclusiveDim(~0) {
+    assert(Values.size() == Dims && "Incorrect number of values");
----------------
NIT: it would be more immediately obvious what this is if you used `std::numeric_limits<unsigned>::max()`


================
Comment at: llvm/include/llvm/Support/TypeSize.h:85
+      Coefficients[I] += RHS.Coefficients[I];
+    verify();
+    return *this;
----------------
I don't think the call to verify() is necessary in any of the arithmetic operators. Just assert that RHS and LHS have the same value set for ExclusiveDim.

You could even special case:

```
if (DimIsExclusive) {
   Coefficients[ExclusiveDim] += RHS.Coefficients[ExclusiveDim];
}
else {
   ...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88982