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

Mon Aug 17 14:30:31 PDT 2020

fpetrogalli added inline comments.

================
Comment at: llvm/include/llvm/Support/TypeSize.h:90
+  /// Return the ElementCount instance representing a scalar.
+  static ElementCount getScalar() { return {1, false}; }
 };
----------------
ctetreau wrote:
> ctetreau wrote:
> > fpetrogalli wrote:
> > > rengolin wrote:
> > > > fpetrogalli wrote:
> > > > > rengolin wrote:
> > > > > > Should we have a `getScalable(int Min)`, `getNonScalable(int Min)` and `getDisabled()` too?
> > > > > > 
> > > > > > There are a number of places you changed from `VF` to `{VF, false}` and places you use literal `{0, false}` that would benefit from having proper names as the static builders. Using them everywhere would make the code safer and clearer to read.
> > > > > > 
> > > > > > Should we have a getScalable(int Min), getNonScalable(int Min) and getDisabled() too?
> > > > > 
> > > > > I'd rather not, for the following reasons. Please let me know if you disagree.
> > > > > 
> > > > > 1.  `getScalable(int Min)`, `getNonScalable(int Min) ` for replacing uses when using `{VF, false}`
> > > > > 
> > > > > The instances of `{VF, false}` are there just because I want to keep this a non-functional change. Those uses are mostly in places were VPlan is computing the possible ElementCount to evaluate. Once we extend VPlan to add also the scalable ECs, we will (likely) just add an extra level of loop iteration on the two values false/true of scalable, and use the ElementCount constructor directly. Once that is done, the two static methods you propose will become obsolete.
> > > > > 
> > > > > I agree that looking at the code with `{VF, false}` may be confusing. Would you prefer me to explicitly call the constructor instead of using an initializer list, so that we know we are dealing with ElementCount?
> > > > > 
> > > > > So, I see two approaches, let me know which one is the best in your opinion:
> > > > > 
> > > > > a. call the constructor explicitly instead of the initializer list
> > > > > b. have  a static method `getFixed` or `getNonScalable` (I prefer the former) that we use but mark it as deprecated as we shouldn't really be using it once the vectorizer doesn't care anymore about scalable and non scalable ECs.
> > > > > 
> > > > > 2. `getDisabled`
> > > > > 
> > > > > I am happy to add a method for the value of 'zero", but calling it `getDisabled` is misleading because ther eis no concept of Enabled/Disabled in ElementCount. I'd prefer to call it `getZero`. I am going this route for this change, let me know if you disagree, will figure something out.
> > > > > 
> > > > > Once we extend VPlan to add also the scalable ECs, we will (likely) just add an extra level of loop iteration on the two values false/true of scalable, and use the ElementCount constructor directly. Once that is done, the two static methods you propose will become obsolete.
> > > > 
> > > > I see what you mean, makes sense.
> > > > 
> > > > 
> > > > > I agree that looking at the code with `{VF, false}` may be confusing. Would you prefer me to explicitly call the constructor instead of using an initializer list, so that we know we are dealing with ElementCount?
> > > > 
> > > > No, init-list constants are usually more readable than explicit constructor calls. The point was not init-list vs c-tor, but constant vs named call, which won't work long term, as you explained above.
> > > > 
> > > > > 2. `getDisabled`
> > > > > 
> > > > > I am happy to add a method for the value of 'zero", but calling it `getDisabled` is misleading because ther eis no concept of Enabled/Disabled in ElementCount. I'd prefer to call it `getZero`. I am going this route for this change, let me know if you disagree, will figure something out.
> > > > 
> > > > LGTM. Thanks!
> > > > 
> > > > No, init-list constants are usually more readable than explicit constructor calls.
> > > 
> > > In the last update of the patch (before your comment) I have replaced the init-list with explicit constructor calls. I just want to make sure you are happy with this. I am am fan of init-list in general, but in this specific case I prefer the constructor, as it make it explicit that we are building something that is used to count the number of elements.
> > My thinking is that people shouldn't be manually constructing ElementCount objects very often. FixedVectorType and ScalableVectorType both have static getters that construct instances using only a number, and vectors have a method to get an ElementCount. I would think code that generically produces vectors that may either be fixed width or scalable from the ground up would not be common.
> > 
> > As for having a deprecated getFixed(), I'd like to point out that we have build bots that build with -werror, so deprecating methods with the intention of using them in upstream isn't going to work out.
> Thinking on this, I kind of like having two static functions `ElementCount::Fixed(unsigned)` and `ElementCount::Scalable(unsigned)`, and hiding all other constructors. This will ensure that, in a hypothetical future where some third kind of vector is added, that code that thinks it's correctly constructing a fixed width ElementCount actually is.
> 
> My previous comment stands, for the most part, only instances of VectorType should be constructing ElementCounts manually. But if some other code needs to, we should make it easy to do right.
I see your point here. If it is OK for you (please confirm), I will create a separate patch that add these two static methods and remove the explicit constructor. For this patch, I have (temporarily) gone for using explicit constructors. 

================
Comment at: llvm/include/llvm/Support/TypeSize.h:87
+  /// One or more elements.
+  bool isVector() const {
+    assert(!(Min == 0 && Scalable) &&
----------------
vkmr wrote:
> 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.
I have removed the constrain in the constructor.

================
Comment at: llvm/include/llvm/Support/TypeSize.h:95
+  /// Return the ElementCount instance representing a scalar.
+  static ElementCount getScalar() { return {1, false}; }
+  /// Return the ElementCount instance representing a zero.
----------------
ctetreau wrote:
> I don't think we should add a getScalar and getZero, because it would be easy to assume these work for scalable vectors end up writing a bunch of bad code. Sure it's an easy fix, but best to just avoid the opportunity for misuse.
I see your point. I have removed `getZero` and `getScalar`.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2287
   unsigned InterleaveFactor = Group->getFactor();
-  auto *VecTy = FixedVectorType::get(ScalarTy, InterleaveFactor * VF);
+  auto *VecTy = FixedVectorType::get(ScalarTy, InterleaveFactor * VF.Min);

----------------
ctetreau wrote:
> assert not scalable
Done, I have also replaced `FixedVectorType` with `VectorType`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794