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

[David Sherwood via Phabricator via llvm-commits]

david-arm 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:
> 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?


================
Comment at: llvm/include/llvm/Support/TypeSize.h:60
   /// Exactly one element.
-  bool isScalar() const { return !Scalable && Min == 1; }
+  bool isScalar() const { return !IsScalable && MinVal == 1; }
   /// One or more elements.
----------------
sdesmalen wrote:
> `isScalar()` only makes sense in the context of ElementCount and doesn't make sense in the context of TypeSize, so I believe this shouldn't live here.
> 
Yes I think you're right. A polynomial definitely can be scalar, but that doesn't mean the scalar value is one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88409