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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the feedback @ctetreau!



================
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) {
----------------
ctetreau wrote:
> 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.
> Is it ever valid for the operator to have a different scalar type than the polynomial?
It is not.

> 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.
Okay I've changed things a bit in the latest update, by using a Traits class to get the ScalarType, and passing the Leaf type (i.e. StackOffset/TypeSize/ElementCount) directly to the base class. That also rendered the `LinearPolyBaseOperators` class unnecessary, as this can now move directly into the base class.

I've also tried adding some `static_assert(std::is_base_of<LinearPolyBase, LeafTy>)`, but that falls over because LeafTy is incomplete at the point where this is being evaluated.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:160
+/// to represent stack offsets.
+class NewStackOffset : public StackOffsetBase,
+                       public LinearPolyBaseOperators<StackOffset, int64_t> {
----------------
ctetreau wrote:
> why "`NewStackOffset`"?
That was to avoid a name clash with `StackOffset` defined in  `AArch64StackOffset.h` when both that header and `TypeSize.h` are included. This got renamed back in D88983. But it's better to put this in a separate namespace, and remove only that namespace in D88983.


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


================
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)};
----------------
ctetreau wrote:
> 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.
The NewStackOffset is indeed a pair, but it's not a `std::pair`, so I thought it would be nice to have this as convenience function. But it's probably better to remove this interface for now, as I don't use it in any of the follow-up patches. 


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


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

https://reviews.llvm.org/D88982