[PATCH] D88409: [SVE] Make ElementCount and TypeSize use a new PolySize class

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:31
+  T MinVal;        // The minimum value that it could be.
+  bool IsScalable; // If true, the total value is determined by multiplying
+                   // 'MinVal' by a runtime determinded quantity, 'vscale'.
----------------
sdesmalen wrote:
> david-arm wrote:
> > ctetreau wrote:
> > > ctetreau wrote:
> > > > david-arm wrote:
> > > > > sdesmalen wrote:
> > > > > > You've implemented PolySize more like the current TypeSize than an actual polynomial. That's fine I guess, but I'm currently looking at some interfaces that could use a general StackOffset. Since that currently lives in the AArch64 target, I might create a follow-up patch that generalises this further using a PolyBase class.
> > > > > Yeah, perhaps I remembered wrong, but I thought this was inline with what we'd discussed previously. This patch is mainly a refactoring effort to introduce the new class and is just the first step in the evolution. It seemed like refactoring + changing the fundamental nature in one patch might be too confusing that's all. For example, the isKnownXY functions will need rewriting to cope with full polynomials. We might even want to audit places where isScalable() is called to ensure the caller knows that returning true doesn't mean there are no fixed width elements. We might want to reverse the question to something like isFixed() perhaps?
> > > > I think it makes sense to implement the actual abstract polynomial, and then have ElementCount and TypeSize inherit from them. So you could have something like:
> > > > 
> > > > ```
> > > > template <typename NumT, typename VarT> class PolynomialTerm {
> > > > private:
> > > >   NumT Coefficient;
> > > >   VarT Var;
> > > >   // TODO: 2 or more vars in one term, assert that this doesn't happen for now
> > > >   // TODO: exponents
> > > > public:
> > > >   NumT getCoefficient() const;
> > > >   void setCoefficient(NumT N);
> > > >   VarT getVar() const;
> > > >   void setVar(VarT V);
> > > > 
> > > > 
> > > >   PolynomialTerm operator*(const PolynomialTerm& N) const;
> > > >   std::vector<PolynomialTerm> operator+(const PolynomialTerm& N) const;
> > > >   // any operator that makes sense for the abstract polynomial term
> > > > 
> > > >   bool isKnownLT(const PolynomialTerm& RHS) const;
> > > >   bool maybeLT(const PolynomialTerm& RHS) const;
> > > >   // all rest of the relations ...
> > > > };
> > > > ```
> > > > 
> > > > And then your ElementCount looks like this:
> > > > 
> > > > ```
> > > > class ElementCount : public PolynomialTerm<unsigned, bool> {
> > > > private:
> > > >   ElementCount(unsigned ElementQuantity, bool Scalable)
> > > >     : PolynomialTerm(ElementQuantity, Scalable) {}
> > > > public:
> > > >   bool isScalable() {
> > > >     return !getVar();
> > > >   }
> > > > 
> > > >   static ElementCount getFixed(unsigned N) {
> > > >     return ElementCount(N, false);
> > > >   }
> > > > 
> > > >   ElementCount divideCoefficientBy(unsigned N) {
> > > >     return ElementCount(getCoefficient() / N, getVar());
> > > >   }
> > > > 
> > > >   // etc... All the functions that make sense for ElementCount but not abstract polynomials
> > > > };
> > > > ```
> > > > 
> > > > This will give us a reusable polynomial that can be extended in the future. If ElementCount and TypeSize are well-behaved, then they will just continue to work if the polynomial is extended in the future.
> > > the implementation of isScalable (should be `return getVar();`) in my example is wrong, but the point stands.
> > Hi @ctetreau I personally only intended this particular patch to be a refactoring effort where we can remove some duplication between the two classes and introduce some level of consistency. This first patch then leaves the door open for the introduction of a real polynomial later on. I know that not everyone is keen on using polynomials for ElementCount and TypeSize so I didn't want to make that decision without a strong consensus. The maths and the comparisons become a lot trickier for true polynomials too, not to mention the interfaces and class structures will need changing. I thought it best to only do that work when and where it was truly needed, and perhaps then we'll have a better understanding of the requirements. Is that ok?
> I'd be okay with you keeping the scope of this patch as-is, I just wanted to let you know I'll probably work on a more generalised polynomial as a follow-up to implement a more generic class for StackOffset. The main concern we wanted to address was removing some duplication, which is what this patch achieves.
> Do please add a TODO comment that explains there is future work required as this class doesn't yet implement a polynomial.
@david-arm Sorry it took me so long to respond.

If this is is meant to be an temporary intermediate step with the goal of reducing code duplication, then I am fine with it. I agree with Sander that a TODO comment describing the current state and ultimate goal of the work should be added.


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

https://reviews.llvm.org/D88409