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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 5 inline comments as done.
fpetrogalli added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:85
+  /// One or more elements.
+  bool isVector() const { return Scalable || Min > 1; }
+  ///@}
----------------
vkmr wrote:
> 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.
Ah - good point! I have added an assertion in the constructor and made the condition here more strict to that {0, true} is not considered a vector. 


================
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);
----------------
vkmr wrote:
> Typo (me -> make)?
Yep, thank you. I have also added an assert as this is a good place where we have to make a conscious choice for enabling scalable vectorization


================
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
----------------
vkmr wrote:
> 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?
Yes, you are correct. I have added this message:

```
assert(!VF.Scalable && "the cost model is not yet implememented for scalable vectorization");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794