[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'.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88409