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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 3 inline comments as done.
fpetrogalli added a comment.

In D85794#2212309 <https://reviews.llvm.org/D85794#2212309>, @rogfer01 wrote:

> 



> We had to do similar things when we applied the LoopVectorizer to RISC-V V-ext. However we didn't use `ElementCount` so our version looks much less neat than your approach.

Out of curiosity, what is your approach? Carrying around a `bool Scalable` flag?

> I think this direction makes a lot of sense.

Thanks!



================
Comment at: llvm/include/llvm/Support/TypeSize.h:71
+  void print(raw_ostream &OS) const {
+    // TODO: this should use sstream
+    if (Scalable)
----------------
rengolin wrote:
> Why?
That's just a leftover. Removed.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:90
+  /// Return the ElementCount instance representing a scalar.
+  static ElementCount getScalar() { return {1, false}; }
 };
----------------
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.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:232
 
-  unsigned BestVF = 0;
+  ElementCount BestVF = ElementCount(0, false);
   unsigned BestUF = 0;
----------------
rengolin wrote:
> Here, a `getDisabled()` would make the code clearer...
I used `getZero`, with a comment saying that 0 EC means vectorization is disabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794