[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:34
 
-// TODO: This class will be redesigned in a later patch that introduces full
-// polynomial behaviour, i.e. the ability to have composites made up of both
-// fixed and scalable sizes.
-template <typename T> class PolySize {
-protected:
-  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'.
+template <typename LeafTy> struct LinearPolyBaseTypeTraits {};
 
----------------
ctetreau wrote:
> Couldn't we just require that any type used as `LeafTy` have a `ScalarTy` typedef?
When I try that, I get:
```error: invalid use of incomplete type 'class llvm::StackOffset'
   55 |   using SomeOtherTy = typename LeafTy::SomeTy;
      |         ^~~~~~~~~~~
```


================
Comment at: llvm/include/llvm/Support/TypeSize.h:158-161
+class StackOffset;
+template <> struct LinearPolyBaseTypeTraits<StackOffset> {
+  using ScalarTy = int64_t;
+};
----------------
ctetreau wrote:
> Does this need to be inside the `NewStackOffset` namespace?  I believe that you are forward declaring `llvm::StackOffset` and then creating the type traits for it, not `llvm::NewStackOffset::StackOffset`
Good catch! I'm not sure why this still built. I can't move the specialization into the NewStackOffset namespace, as this needs to happen in the same namespace as it's declaration, but I can move the forward declaration of StackOffset into a namespace, and specify the traits for `NewStackOffset::StackOffset`.


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

https://reviews.llvm.org/D88982