[PATCH] D47477: [VPlan] Avoid building VPlans for each VF in the ILV (NFC).

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

Thanks for the patch, Florian! Ok, now I understand what you mean exactly. Some comments inline.



================
Comment at: lib/Transforms/Vectorize/LoopVectorizationPlanner.h:352
+  /// Legal. This method is only used for the legacy inner loop vectorizer.
+  VPlanPtr buildVPRecipes(VFRange &Range);
 };
----------------
I really like this change! This is decoupling the VPlan construction with VPInstructions from the legacy VPlan construction using recipes. I understand that you will refactor even more the code in `buildVPRecipes` to implement the temporary VPInstruction-to-VPRecipe step in the native path.

Maybe we should name this buildVPlanWithRecipes or something like that to clearly state that it's creating a new VPlan?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6342
 
-  buildVPlans(1, MaxVF);
+  VFRange R = {BestVF.Width, BestVF.Width + 1};
+  VPlans.push_back(buildVPRecipes(R));
----------------
Why `BestVF.Width + 1`? Shouldn't be `BestVF.Width`?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6347
   // Select the optimal vectorization factor.
-  return CM.selectVectorizationFactor(MaxVF);
+  return BestVF;
 }
----------------
The rationale of building VPlans for all the VFs and then invoke CM to choose the best VF (without using those VPlans) was to eventually port CM to work on these VPlans. This hasn't happened yet but @hsaito is working on this VPlan based version of the CM. We think that this step is very important in the converge of both vectorization paths since it will allow us to move the VPlan construction in the inner loop path to earlier stages of the path.

Since this is a small change, my suggestion would be: 1) if this change is needed by your subsequent patches, let's go with it. Hideki could move the VPlan construction again after CM when his code is ready; 2) otherwise, let's keep the original version. Please, note that we are mostly sharing a single VPlan for all the VFs so we shouldn't be using unnecessary memory most of the times.

WDYT?



https://reviews.llvm.org/D47477