[PATCH] D32200: [LV] Refactor ILV.vectorize[Loop]() by introducing LVP.executePlan()

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:394
 
-  // Perform the actual loop widening (vectorization).
-  void vectorize() {
-    // Create a new empty loop. Unlink the old loop and connect the new one.
-    createEmptyLoop();
-    // Widen each instruction in the old loop to a new one in the new loop.
-    vectorizeLoop();
-  }
+  /// Create a new empty loop. Unlink the old loop and connect the new one.
+  void createVectorizedLoop();
----------------
rengolin wrote:
> This also vectorises, no? This comment implies it just replaces with an empty loop.
Ahh, no, it creates an empty loop into which vector code will subsequently be inserted (by calls to vectorizeInstruction()), and introduces its scalar induction variable of step VF*UF. It also creates all the surrounding blocks, branches, etc.. This was the exact original behavior of the method, and its original comment. This patch only exposes it publicly, to be (re)used for generating code by vplan.

Perhaps a better yet name for it may be createVectorizedLoopSkeleton(). Can we agree on this, or other welcomed suggestions?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7531
+void LoopVectorizationPlanner::executePlan(InnerLoopVectorizer &ILV) {
+  // Perform the actual loop widening (vectorization).
+
----------------
rengolin wrote:
> This is not strictly true, as it could be unrolling, right?
> 
> Maybe you should say "loop transformation"?
Right, executing a Plan follows the selected VF and UF, which may end up producing vector and/or unrolled code.

This is the original comment taken from ILV::vectorize(), which also serves the Unroller.

Agreed, we should say "loop transformation" instead.


https://reviews.llvm.org/D32200