[PATCH] D32871: [LV] Using VPlan to model the vectorized code and drive its transformation

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

A few more comments... :)



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:470
 
+public:
   /// A helper function that computes the predicate of the block BB, assuming
----------------
Ayal wrote:
> rengolin wrote:
> > The public protected swap is confusing. I'd create a "protected bubble" and move the methods inside it.
> Sorry for this confusion. The idea was to avoid moving methods around to simplify the diff:
> 
> > Protected methods made public to enable their reuse are annotated as such in-place to simplify the diff. They will eventually be moved and grouped together.
> 
> An alternative is to use "friend" instead.
Grouping should be fine.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:497
+  /// inclusive..
+  void scalarizeInstruction(Instruction *Instr, const VPIteration &Instance,
+                            bool IfPredicateInstr);
----------------
Ayal wrote:
> rengolin wrote:
> > Not virtual anymore? Should it be marked "final"?
> This should have been part of r297580 originally.
> 
> In general ILV's methods are marked virtual iff they're overridden by Unroller; otherwise they may be marked "final".
> 
> This is unrelated to VPlan. Sorry for the confusion.
Right, a separate fix is better in this case, then.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:720
 
+    ScalarParts &getOrCreateScalar(Value *Key, unsigned Lanes) {
+      if (!hasScalar(Key)) {
----------------
A short comment would be nice, here.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2263
+
+  ~LoopVectorizationPlanner() {
+    while (!VPlans.empty()) {
----------------
Nit: Can you move the member variables to the top? Makes it easier to know what "VPlans" are, etc.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2265
+    while (!VPlans.empty()) {
+      VPlan *Plan = VPlans.back();
+      VPlans.pop_back();
----------------
SmallVectorImpl should destroy its own range, you don't need to do it yourself.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2298
+    unsigned Start; // A power of 2.
+    unsigned &End;  // Need not be a power of 2. If End <= Start range is empty.
+  };
----------------
This POD is so small that it's worth passing it by value most of the time. If you need to change it, it's also worth passing it by reference as a whole.


https://reviews.llvm.org/D32871