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

[Renato Golin via Phabricator via llvm-commits]

rengolin 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}; }
 };
----------------
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!



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794