[PATCH] D90342: [POC][LoopVectorizer] Propagate ElementCount to interfaces in preparation for scalable auto-vec.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked 4 inline comments as done.
sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:352
 
+  LeafTy inc(ScalarTy Amount) const {
+    return static_cast<LeafTy>(
----------------
ctetreau wrote:
> I feel like a function called "inc" should mutate the thing being incremented. Morally, this is behaves like `operator+=`.
> 
> All uses currently of this function would be fine with these semantics with some small changes (that should probably be done anyways)
You're right that `inc` suggests the object itself is modified. However, to my opinion that results in some odd looking code, like:

```MaxVF.inc(1); // Implicitly modifies MaxVF
for (ElementCount VF = MinVF; ElementCount::isKnownLT(VF, MaxVF); ) { ... }
```

I've pulled this change out into a separate patch, D90713, where I've added `add` and `sub` methods that return a new object.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7846
   RSO << "Initial VPlan for VF={" << VF;
-  for (VF *= 2; VF.getKnownMinValue() < Range.End; VF *= 2) {
+  assert(VF.isScalable() == Range.End.isScalable() &&
+         "Scalable flags don't match");
----------------
vkmr wrote:
> Do we need this assert here?  Wouldn't it be redundant with the assert in the VFRange constructor?
We don't, good spot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90342