[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:35
+/// yet returning the type of the derived class.
+template <class Ty, typename ScalarTy> class LinearPolyBaseOperators {
+  friend Ty &operator+=(Ty &LHS, const Ty &RHS) {
----------------
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.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:77
+/// when false.
+template <typename T, unsigned Dimensions, bool IsUnivariate>
+class LinearPolyBase {
----------------
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})`.


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

https://reviews.llvm.org/D88982