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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

I think this is a good direction, too.

I don't like implicit assumptions or magic constants, especially when the two (bool scalable and int min) are not linearly independent.

The fact that the patch is huge doesn't make much difference when it's mostly mechanical.

There may be some leftovers, so I suggest you do a grep for `Min >` and variations, to make sure you got all of them.

I have some comments inline but am mostly happy with it.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:71
+  void print(raw_ostream &OS) const {
+    // TODO: this should use sstream
+    if (Scalable)
----------------
Why?


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



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:232
 
-  unsigned BestVF = 0;
+  ElementCount BestVF = ElementCount(0, false);
   unsigned BestUF = 0;
----------------
Here, a `getDisabled()` would make the code clearer...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794