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

[Vineet Kumar via Phabricator via llvm-commits]

vkmr added a comment.

Thanks @fpetrogalli for this proposal! I agree with @rogfer01 and @rengolin in that this approach makes a lot of sense.
I have added a few comments inline.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:85
+  /// One or more elements.
+  bool isVector() const { return Scalable || Min > 1; }
+  ///@}
----------------
Unless there is something explicitly preventing from constructing a `{0, true}` `ElementCount`, this could potentially hide bugs by returning `true` for this case. Constraining `ElementCount` constructor is probably a better way.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2725
   Type *Ty = TC->getType();
-  Constant *Step = ConstantInt::get(Ty, VF * UF);
+  // This is where we can me the step a runtime constant.
+  Constant *Step = ConstantInt::get(Ty, VF.Min * UF);
----------------
Typo (me -> make)?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6195
+                                               ElementCount VF) {
+  assert(!VF.Scalable);
   // If we know that this instruction will remain uniform, check the cost of
----------------
Missing message for `assert`.
I believe this assert falls under point 4 (When building the potential VFs in VPlan. Making the VPlan generic) in your description of where it would be a functional change to support scalable vectors, right?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6225
 
-  if (VF == 1)
+  assert(!VF.Scalable);
+  if (VF.isScalar())
----------------
Missing message in `assert`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794