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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal marked 5 inline comments as done.
Ayal added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:470
 
+public:
   /// A helper function that computes the predicate of the block BB, assuming
----------------
rengolin wrote:
> 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.
Done.

Also desired for "public"-only changes to CostModel methods?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:497
+  /// inclusive..
+  void scalarizeInstruction(Instruction *Instr, const VPIteration &Instance,
+                            bool IfPredicateInstr);
----------------
rengolin wrote:
> 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.
Dropping 'virtual' from scalarizeInstruction() was committed separately.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2265
+    while (!VPlans.empty()) {
+      VPlan *Plan = VPlans.back();
+      VPlans.pop_back();
----------------
rengolin wrote:
> SmallVectorImpl should destroy its own range, you don't need to do it yourself.
Leaving ~LoopVectorizationPlanner() {} does not seem to deallocate the VPlan objects created by buildVPlan(). Is there another way to prevent them from leaking?


================
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.
+  };
----------------
rengolin wrote:
> 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.
Holding a constant Start plus non-constant End, and passing the struct by reference wherever End may be modified, instead of holding an unsigned Start and an unsigned reference End.


https://reviews.llvm.org/D32871