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

Wed Oct 21 10:16:45 PDT 2020

ctetreau added inline comments.

================
Comment at: llvm/include/llvm/Support/TypeSize.h:35
+/// yet returning the type of the derived class.
+template <class Ty, typename ScalarTy> class LinearPolyBaseOperators {
+  friend Ty &operator+=(Ty &LHS, const Ty &RHS) {
----------------
sdesmalen wrote:
> david-arm wrote:
> > Can you remove the need for ScalarTy by having Ty provide the scalar type? i.e. Ty::ScalarTy, where ScalarTy would be a typedef or something inside LinearPolyBase?
> > 
> > Instead of using multiple inheritance to get the operators, is it not possible to simply have the operators outside of any class? I think C++ allows you to do this provided the operators are friends of the relevant classes, i.e. just have
> > 
> > template <Ty>
> > LinearPolyBase<Ty> operator+(const LinearPolyBase<Ty> LHS, const LinearPolyBase<Ty> RHS) {
> >   for (unsigned I = 0; I < Dimensions; ++I)
> >     LHS.Coefficients[I] -= RHS.Coefficients[I];
> >   return LHS;
> > }
> > 
> > although I realise the example above only works if you have a simplified LinearPolyBase that assumes 2 dimensions, rather than the 3 template parameters you currently have for LinearPolyBase.
> > Instead of using multiple inheritance to get the operators, is it not possible to simply have the operators outside of any class
> That is kind of what this class tries to do. It defines the functions as friend functions, where it takes the result/operand types from the derived class, like TypeSize, ElementCount or StackOffset.
> 
> The problem with using inheritance for the operators is that it inherits the parent's operators, which return the parent class' type. That requires implicit conversion of parent -> derived class. By inheriting from this class like this:
> 
> ```class ElementCount : public LinearPolyBase<...>,
>                      public LinearPolyBaseOperators<ElementCount, ...> {
> };```
> 
> it will automatically instantiate those operators for the derived class.
> 
Is it ever valid for the operator to have a different scalar type than the polynomial?

If not, then I agree with David that LinearPolyBase should publicly expose a `using ScalarType = T`, and that this type should only 1 template parameter that is the polynomial type.

It might also be nice to do some type_traits magic to static_assert that the polynomial type actually inherits from LinearPolyBase so the user doesn't get 20 pages of template garbage if they mess it up.

================
Comment at: llvm/include/llvm/Support/TypeSize.h:77
+/// when false.
+template <typename T, unsigned Dimensions, bool IsUnivariate>
+class LinearPolyBase {
----------------
ctetreau wrote:
> sdesmalen wrote:
> > david-arm wrote:
> > > Do we actually need to support this right now? I just wonder if we can simplify things by just assuming it's always univariate for now. I guess I'm just worried that we're trying to provide theoretical support for something with no proof as yet that it's needed. We're always going to be targeting one backend, so when switching backends vscale will mean whatever is appropriate for the given backend. I think this code is trying to support a single backend having multiple vscales, unless I've misunderstood something?
> > > 
> > > Also, what happens when the user specifies 3 dimensions and IsUnivariate=true? I guess this is technically allowed, but looks a little odd that's all.
> > > 
> > > This is just a suggestion, but I wonder if the code might be made simpler by always assuming coefficient 0 is unscaled. Then 2 dimensions always implies both univariate and that coefficient 1 is scaled? By definition, 3 dimensions automatically implies IsUnivariate=false too I think? In this case you may actually only need two template parameters. Also, if we assume for now we're always dealing with exactly two dimensions then the enable_if stuff goes away too as then you only need one two-arg constructor.
> > > Do we actually need to support this right now? I just wonder if we can simplify things by just assuming it's always univariate for now.
> > We need this for StackOffset, which has IsUnivariate = false, because it needs to support a combination of fixed and scalable elements.
> > 
> > > I think this code is trying to support a single backend having multiple vscales, unless I've misunderstood something?
> > I've made it generic enough in case this is needed in the future, mostly because it didn't really make things more complicated. I guess we could fix the `Dimensions` to 2, and remove this template operand from the class and explicitly name these dimensions 'Fixed' and 'Scalable', although I'm not sure if that would actually simplify these classes much.
> > 
> > > Also, what happens when the user specifies 3 dimensions and IsUnivariate=true? I guess this is technically allowed, but looks a little odd that's all.
> > > This is just a suggestion, but I wonder if the code might be made simpler by always assuming coefficient 0 is unscaled. Then 2 dimensions always implies both univariate and that coefficient 1 is scaled? By definition, 3 dimensions automatically implies IsUnivariate=false too I think? In this case you may actually only need two template parameters
> > The dimensions don't have a meaning in this class, it's just a representation for: `c0 * scale0 + c1 * scale1 + ... + cK * scaleK`. This abstract class could represent a K dimensionsal linear polynomial, where IsUnivariate would ensure that only one of {c0, c1, ..., ck} is non-zero.
> > 
> > > Also, if we assume for now we're always dealing with exactly two dimensions then the enable_if stuff goes away too as then you only need one two-arg constructor
> > The enable_if is there to distinguish constructors for the Univariate and non-Univariate cases. i.e. If only one dimension is allowed to be set, you call it with `(cK, #dimK)`, otherwise you call it with `({c0, c1, ..., cK})`.
> I like the template parameter for the number of different variables, and would prefer that we didn't fix it to 2. I think it doesn't really increase the complexity, and makes supporting more in a hypothetical future where some target has multiple vscale like things much easier. It is my personal opinion that N+1 template parameters aren't more complex than N template parameters unless N is zero.
NIT: add static_assert that Dimensions is not unsigned max

================
Comment at: llvm/include/llvm/Support/TypeSize.h:160
+/// to represent stack offsets.
+class NewStackOffset : public StackOffsetBase,
+                       public LinearPolyBaseOperators<StackOffset, int64_t> {
----------------
why "`NewStackOffset`"?

================
Comment at: llvm/include/llvm/Support/TypeSize.h:161
+class NewStackOffset : public StackOffsetBase,
+                       public LinearPolyBaseOperators<StackOffset, int64_t> {
+protected:
----------------
Shouldn't the first template parameter be `NewStackOffset`?

================
Comment at: llvm/include/llvm/Support/TypeSize.h:177
+  int64_t getScalable() const { return this->getValue(1); }
+  std::pair<int64_t, int64_t> getMixed() const {
+    return {this->getValue(0), this->getValue(1)};
----------------
What's the point of this? NewStackOffset is already basically a pair of int64_t.

Also, the user must read the implementation to know which member of the pair is the fixed coefficient and which is the scalable one. At the very least, this should be documented.

================
Comment at: llvm/include/llvm/Support/TypeSize.h:192
+                       public LinearPolyBaseOperators<LinearPolySize<T>, T> {
+protected:
+  LinearPolySize(T MinVal, bool IsScalable)
----------------
If we name these dims, we can just use them instead of mapping bool -> unsigned

================
Comment at: llvm/include/llvm/Support/TypeSize.h:193-194
+protected:
+  LinearPolySize(T MinVal, bool IsScalable)
+      : LinearPolySizeBase<T>(MinVal, !IsScalable ? 0 : 1) {}
+
----------------

================
Comment at: llvm/include/llvm/Support/TypeSize.h:199-200
+      : LinearPolySizeBase<T>(Other) {}
+  static LinearPolySize getFixed(T MinVal) { return {MinVal, false}; }
+  static LinearPolySize getScalable(T MinVal) { return {MinVal, true}; }
+  static LinearPolySize get(T MinVal, bool Scalable) {
----------------

================
Comment at: llvm/include/llvm/Support/TypeSize.h:202
+  static LinearPolySize get(T MinVal, bool Scalable) {
+    return {MinVal, Scalable};
+  }
----------------

================
Comment at: llvm/include/llvm/Support/TypeSize.h:208
+  /// Returns whether the size is scaled by a runtime quantity (vscale).
+  bool isScalable() const { return this->UnivariateDim == 1; }
   /// A return value of true indicates we know at compile time that the number
----------------

================
Comment at: llvm/unittests/Support/StackOffsetTest.cpp:1
+//===- TestNewStackOffset.cpp - NewStackOffset unit tests------------------------===//
+//
----------------
We should probably just write unit tests for LinearPolyBase, and get rid of tests for ElementCount, TypeSize, and StackOffset unless they test actual new features.

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

https://reviews.llvm.org/D88982