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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Hi Ayal,

I like where this is going, particularly the `executePlan` idiom. I don't think it's a really big deal now how we organise the interface, given that it's going to change (probably radically) once VPlans start to trickle in.

A few silly comments on comments, but otherwise, it's looking good. Thanks!

I'll let @mssimpso and @mkuper finish their reviews. :)

cheers,
--renato



================
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();
----------------
This also vectorises, no? This comment implies it just replaces with an empty loop.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7531
+void LoopVectorizationPlanner::executePlan(InnerLoopVectorizer &ILV) {
+  // Perform the actual loop widening (vectorization).
+
----------------
This is not strictly true, as it could be unrolling, right?

Maybe you should say "loop transformation"?


https://reviews.llvm.org/D32200