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

Mon Aug 17 16:14:30 PDT 2020

ctetreau added a comment.

What you have so far seems like a good approach. I'll refain from giving approval as this is still WIP and other people should give it a look. It might be easier to get it in after D86065 <https://reviews.llvm.org/D86065> and any follow patches to that land anyways.

================
Comment at: llvm/include/llvm/Support/TypeSize.h:90
+  /// Return the ElementCount instance representing a scalar.
+  static ElementCount getScalar() { return {1, false}; }
 };
----------------
fpetrogalli wrote:
> 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. 
That's fine. It probably makes sense to do this either in D86065 or as a follow up to that patch. The way I see it, this work is about "using ElementCount" and David's is about "improving ElementCount"

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794