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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked 6 inline comments as done.
sdesmalen added a comment.

Okay, I have addressed all comments. Specializing the two base classes for Mixed and Univariate cases and moving the dimension to the Traits class improves the code quite a bit.
Let me know what you think!



================
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;
----------------
ctetreau wrote:
> 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...
Using a linear algebra lib is probably a bit overkill here as the few supported operations are simple enough to implement. Unless there is already something in LLVM for this?


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

https://reviews.llvm.org/D88982