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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked an inline comment as done.
sdesmalen added a comment.

FYI - This patch has been split up into D90713 <https://reviews.llvm.org/D90713>, D90715 <https://reviews.llvm.org/D90715>, D90879 <https://reviews.llvm.org/D90879> and D90880 <https://reviews.llvm.org/D90880>.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:352
 
+  LeafTy inc(ScalarTy Amount) const {
+    return static_cast<LeafTy>(
----------------
ctetreau wrote:
> 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
Thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7006
+    buildVPlansWithVPRecipes(UserVF,
+                             UserVF);
     LLVM_DEBUG(printPlans(dbgs()));
----------------
hiraditya wrote:
> formating...
Thanks, this is addressed in D90879.


================
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:
> 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.
This has been fixed in D90715. Sorry, I just realised that I forgot you as a reviewer to that patch.


================
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:
> ctetreau wrote:
> > I'm pretty sure this inc can be moved outside also
> still worth doing IMO
Also fixed in D90715.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90342