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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:352
 
+  LeafTy inc(ScalarTy Amount) const {
+    return static_cast<LeafTy>(
----------------
sdesmalen wrote:
> 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.
your other patch has been approved


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7202-7209
+void LoopVectorizationPlanner::buildVPlans(ElementCount MinVF,
+                                           ElementCount MaxVF) {
+  for (ElementCount VF = MinVF; ElementCount::isKnownLT(VF, MaxVF.inc(1));) {
+    VFRange SubRange = {VF, MaxVF.inc(1) };
     VPlans.push_back(buildVPlan(SubRange));
     VF = SubRange.End;
   }
----------------
ctetreau wrote:
> MaxVF.inc(1) can be moved outside the loop
I still think this change is worth making, even given the fact that inc() has not been made to mutate the object.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7661-7662
 
-  for (unsigned VF = MinVF; VF < MaxVF + 1;) {
-    VFRange SubRange = {VF, MaxVF + 1};
+  for (ElementCount VF = MinVF; ElementCount::isKnownLT(VF, MaxVF.inc(1));) {
+    VFRange SubRange = {VF, MaxVF.inc(1)};
     VPlans.push_back(buildVPlanWithVPRecipes(SubRange, NeedDef,
----------------
ctetreau wrote:
> I'm pretty sure this inc can be moved outside also
still worth doing IMO


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90342