[PATCH] D85794: [WIP][llvm][LV] Replace `unsigned VF` with `ElementCount VF` [NFCI]

[Vineet Kumar via Phabricator via llvm-commits]

vkmr added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:87
+  /// One or more elements.
+  bool isVector() const {
+    assert(!(Min == 0 && Scalable) &&
----------------
ctetreau wrote:
> fpetrogalli wrote:
> > vkmr wrote:
> > > ctetreau wrote:
> > > > Constraining the constructor doesn't help with the case where a valid `ElementCount` is constructed, and then `Min` set to zero later on.
> > > > 
> > > > Why exactly is `{true, 0}` forbidden? `forall N, N * 0 = 0` after all.
> > > @ctetreau I see your point about constraining the constructor for scalable vectors; `ElementCount` is a counter and 0 is a valid count. Also, however one defines a "vector with 0 elements", it would be the same for fixed and scalar vectors. So, IF the constructor allows constructing  fixed vectors of 0 length, it should also allow scalable vectors of 0 length.
> > > 
> > > The perspective behind not allowing to have `Min = 0` (for both fixed and scalable vectors) is that `ElementCount`'s only purpose is to count the number of elements in a vector, and a vector with no elements is illegal vector. But that discussion would be out of scope of this patch.
> > > 
> > > The updated condition for `isVector()` still makes sense though.
> > 
> > Mathematically you are right. ElementCount is  a monomial of grade 0 (a*X^0) or 1 (a*X^1). It doesn't really matter if you are multiplying by 0*X^0 or 0*X^1, both values will null the result of the operation independently of the EC in input.
> > 
> > I think that @vkmr was mostly concerned about potential bugs that can happen when we check if we are working in scalable mode (so Scalable == true) but we forget to check that we have actually a non zero value by checking Min > 0. It is very unlikely to happen, but I guess there is some argument for choosing this approach. 
> > 
> > All in all, I'd rather avoid `{0, true}`, at list in debug mode, just to catch things that might end up weird if we end up producing this value.
> So you're telling me that there's no good reason to construct a {0, true}, so as a debugging aid you're asserting that it doesn't happen? Why is {0, false} OK? I could see some valid code doing something like:
> 
> ```
> ElementCount EC = {std::min(someQuantity, someOtherQuantity), true};
> if (!EC.isVector())
>    return false;
> ```
> 
> Sure, I could delay construction of the ElementCount, but there's no good reason that I should have to do that.
> 
> Really though, as long as the Min and Scalable fields are public, I don't think we should try to preserve any invariants for them. They should either be made private (probably outside the scope of what you are trying to do), or its functionality should be expected to work for any combination of {Min, Scalable}.
@fpetrogalli, I saw your reply after writing my comment. 
My main concern was just the condition in `isVector()`, which in the original draft was `return (Scalable  || Min > 1;`, that would incorrectly report `{0, true}` to be a vector, (but not `{0, false}`.  
As I mentioned in the other comment, for this patch I would prefer a consistent behavior for fixed and scalable vectors where it makes sense. We can remove the `asserts` for `"invalid values: zero can not be scalable"`. Also `isZero() ` works for both, so I am not too worried.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794