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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:56-57
 
-  static constexpr PolySize getFixed(T MinVal) { return {MinVal, false}; }
-  static constexpr PolySize getScalable(T MinVal) { return {MinVal, true}; }
-  static constexpr PolySize get(T MinVal, bool IsScalable) {
-    return {MinVal, IsScalable};
+protected:
+  std::array<ScalarTy, Dimensions> Coefficients;
+  unsigned UnivariateDim;
----------------
georges wrote:
> Is it worth specializing this struct template for IsUnivariate=true/false, since with the number of enable_if's scattered around they begin to look like quite different classes? It also seems a bit wasteful to be allocating Dimensions elements when all but one of them will be zero in the Univariate case?
@ctetreau do you have any thoughts on this? I guess I could split this up and end up with a two base classes, as I agree with @georges that for >2 dimensions the code isn't optimal. By having two base classes, the univariate case can be optimized. The downside is some duplication, but it would avoid the need for `enable_if` and probably simplify the code.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:125-128
+  bool operator==(const LinearPolyBase &RHS) const {
+    return std::equal(&Coefficients[0], &Coefficients[Dimensions],
+                      &RHS.Coefficients[0]);
+  }
----------------
ctetreau wrote:
> Out of bounds array access.
Eek, good spot!


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

https://reviews.llvm.org/D88982