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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.
Herald added a subscriber: dexonsmith.

Looks good to me once you add that assert.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:180
+
+  UnivariateLinearPolyBase(ScalarTy &Val, unsigned UnivariateDim)
+      : Value(Val), UnivariateDim(UnivariateDim) {}
----------------
georges wrote:
> worth an assert that UniveriateDim is in range?
I think this would be a useful assert


================
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:
> 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?
Yeah, that was my thought too, which is why I didn't actually suggest it. I'm surprised we don't have a linear algebra lib already (either homegrown or not) though. Seems like it'd be useful for doing things like constexpr matrix multiplications.


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

https://reviews.llvm.org/D88982