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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper accepted this revision.
mkuper added a comment.
This revision is now accepted and ready to land.

I think the revision summary doesn't fully represent what's going on here at this point. :-)

In any case, I agree with Matt and Renato - this looks good to go, we can continue iterating on it in-tree.



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2324
+  /// returned value holds for the entire \p Range.
+  bool testVFRange(const std::function<bool(unsigned)> &Predicate,
+                   VFRange &Range);
----------------
Ayal wrote:
> mkuper wrote:
> > This seems like a somewhat odd API.
> > Why do we only support cutting from the top, and not from the bottom or the middle? Do we expect to only ever prune from the top? Otherwise, I'd expect the range to be represented as a set, rather than an interval, and use a filter over that set. The current state may be fine for simplicity's sake, but i'd like to understand this better.
> > 
> > Regardless, please rename the method. It's really surprising that a bool test...() modifies one of its arguments.
> Indeed a more general implementation of building VPlans could represent arbitrary sets of VF's rather than "Ranges" or intervals of VF's, which this patch uses. VPlans themselves are not confined to represent only ranges.
> 
> This implementation builds VPlans for the full {1,2,4,8,...,MaxVF} range of feasible VF's by repeatedly building a VPlan starting from a given VF up until the maximum VF possible. Each vectorization decision can potentially reduce this maximum. The two extremes we could end up with are: one VPlan for all VF's, and one VPlan for each VF. Decisions hopefully exhibit this form of continuity, but they certainly don't have to.
> 
> Will see if above should be added as a comment to buildVPlans().
> 
> 
> Some alternative names we came up with:
> ```
> testAndClampVFRange()
> prefixTestVFRange()
> VFRangePrefixTest()
> testStartAndSetEndVF()
> ```
> any one of these looks ok? Better suggestions are welcome.
Ok, this still sounds odd, but we can iterate over this in-tree in the future.
Re naming, testAndClampVFRange() sounds better, I think, but I'm good with anything that indicates this changes the range.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7750
 
+namespace {
+/// VPWidenRecipe is a recipe for producing a copy of vector type for each
----------------
Ayal wrote:
> mkuper wrote:
> > Maybe break all of the recipe stuff out into a separate file? Two files, even? (header/implementation)
> Yes, this large LoopVectorizer.cpp file should be broken into multiple smaller files. ILV in its current form hinders doing so with these recipes. Follow-up patches should help facilitate this desired change.
It seems like putting all the recipes in a separate file would be easy to start with (instead of going into an anonymous namespace here.)
If it isn't, I'm ok with doing this in follow-ups.


https://reviews.llvm.org/D32871