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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

@sdesmalen I'm generally ok with this approach. This patch is pretty complicated, so I haven't had the time to fully digest it and decide if it's actually correct. I plan to try to find time to do this soon, but I'd feel more comfortable if others took a look as well.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:77
+/// when false.
+template <typename T, unsigned Dimensions, bool IsUnivariate>
+class LinearPolyBase {
----------------
sdesmalen wrote:
> david-arm wrote:
> > Do we actually need to support this right now? I just wonder if we can simplify things by just assuming it's always univariate for now. I guess I'm just worried that we're trying to provide theoretical support for something with no proof as yet that it's needed. We're always going to be targeting one backend, so when switching backends vscale will mean whatever is appropriate for the given backend. I think this code is trying to support a single backend having multiple vscales, unless I've misunderstood something?
> > 
> > Also, what happens when the user specifies 3 dimensions and IsUnivariate=true? I guess this is technically allowed, but looks a little odd that's all.
> > 
> > This is just a suggestion, but I wonder if the code might be made simpler by always assuming coefficient 0 is unscaled. Then 2 dimensions always implies both univariate and that coefficient 1 is scaled? By definition, 3 dimensions automatically implies IsUnivariate=false too I think? In this case you may actually only need two template parameters. Also, if we assume for now we're always dealing with exactly two dimensions then the enable_if stuff goes away too as then you only need one two-arg constructor.
> > Do we actually need to support this right now? I just wonder if we can simplify things by just assuming it's always univariate for now.
> We need this for StackOffset, which has IsUnivariate = false, because it needs to support a combination of fixed and scalable elements.
> 
> > I think this code is trying to support a single backend having multiple vscales, unless I've misunderstood something?
> I've made it generic enough in case this is needed in the future, mostly because it didn't really make things more complicated. I guess we could fix the `Dimensions` to 2, and remove this template operand from the class and explicitly name these dimensions 'Fixed' and 'Scalable', although I'm not sure if that would actually simplify these classes much.
> 
> > Also, what happens when the user specifies 3 dimensions and IsUnivariate=true? I guess this is technically allowed, but looks a little odd that's all.
> > This is just a suggestion, but I wonder if the code might be made simpler by always assuming coefficient 0 is unscaled. Then 2 dimensions always implies both univariate and that coefficient 1 is scaled? By definition, 3 dimensions automatically implies IsUnivariate=false too I think? In this case you may actually only need two template parameters
> The dimensions don't have a meaning in this class, it's just a representation for: `c0 * scale0 + c1 * scale1 + ... + cK * scaleK`. This abstract class could represent a K dimensionsal linear polynomial, where IsUnivariate would ensure that only one of {c0, c1, ..., ck} is non-zero.
> 
> > Also, if we assume for now we're always dealing with exactly two dimensions then the enable_if stuff goes away too as then you only need one two-arg constructor
> The enable_if is there to distinguish constructors for the Univariate and non-Univariate cases. i.e. If only one dimension is allowed to be set, you call it with `(cK, #dimK)`, otherwise you call it with `({c0, c1, ..., cK})`.
I like the template parameter for the number of different variables, and would prefer that we didn't fix it to 2. I think it doesn't really increase the complexity, and makes supporting more in a hypothetical future where some target has multiple vscale like things much easier. It is my personal opinion that N+1 template parameters aren't more complex than N template parameters unless N is zero.


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

https://reviews.llvm.org/D88982