[PATCH] D88982: [NFCI] Add LinearPolyBase 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: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;
----------------
sdesmalen wrote:
> 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.
I'm not so much worried about wasted space because Dimensions is likely to be low.

However, my general feeling is that this class getting way too fancy with the template metaprogramming. I wasn't going to complain about it, but since you're asking I think it would be more straightforward if they were separate classes.

I've also wondered on more than on occasion if we ought to just use a linear algebra lib for this. This class smells a lot like a vector to me...


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

https://reviews.llvm.org/D88982