[PATCH] D42447: [LV][VPlan] Detect outer loops for explicit vectorization.

[Diego Caballero via Phabricator via llvm-commits]

dcaballe marked 4 inline comments as done.
dcaballe added a comment.

Thanks you, Renato and Florian, for your comments.

> this limitation is due to the fact that VPlan based cost-modelling is not implemented yet, right?

Not only. For full outer loop auto-vectorization we'd also need to extend Legal to check for data dependences that prevent the vectorization of outer loops. In loop nests with several outer loops, we'd also need to compare the cost of vectorizing each of them.

> I think for testing, it would useful to have an option to process all outer loops. The legality checks should filter out any unsupported loops and this way we could test the VPlan native code path on a much wider range of loops. I think it also would be great if we would have a bot that runs at least the test-suite with VPlan native to discover regressions.

Is -vplan-build-stress-test flag in Patch #3 (https://reviews.llvm.org/D44338) aligned with what you had in mind? :)
I would need some help/guidance with the bot part since I'm not familiar with that.



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:29
 //
+// There is a development effort going on to migrate loop vectorizer to the
+// VPlan infrastructure and to introduce outer loop vectorization support (see
----------------
fhahn wrote:
> Is it worth mentioning docs/Proposal/VectorizationPlan.rst as well?
Definitely. Thanks!


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:263
+static cl::opt<bool> EnableVPlanNativePath(
+    "enable-vplan-native-path", cl::init(false), cl::Hidden,
+    cl::desc("Enable VPlan-native vectorization path with "
----------------
rengolin wrote:
> Right now, this is just enabling outer-loop. Are you planning on adding more functionality to the native part of VPlan before merging the inner loop vectoriser into it? I wouldn't recommend, as we really don't want two paths in parallel for too long.
> 
> I'd recommend this to just be called "vectorize-outerloop" or something.
Inner loop vectorization is a subset of outer loop vectorization so the VPlan native path will be inherently supporting inner loops. However, it's not our intention to enable it "for production" while both paths co-exist. However, as we described in the RFC, inner loop vectorization support in the VPlan native path is indispensable for the convergence of both paths. As we start migrating and extending all the existing functionality for inner loops to outer loops in the VPlan native path, we will need to compare side-by-side where both paths stand regarding inner loop vectorization. When both paths are comparable in that regard, the migration will be completed.

Inner loop support will also be very useful for (stress) testing the VPlan native path, since some loops don't have another loop around.

For these reasons we are not using the 'outerloop' word in the flags/interfaces.
Does it make sense to you?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7650
+    // TODO: VPlan-based Predication.
+    // TODO: VPlan-based Cost Modeling.
+
----------------
rengolin wrote:
> dcaballe wrote:
> > rengolin wrote:
> > > is this really the place to do predication and cost modelling?
> > This function returns the VF to be used during code generation so we would need to evaluate the cost here to return the selected VF. Cost modeling shouldn't be part of the VPlan building process. The same approach is followed in the code below. Regarding predication, it must happen before the cost evaluation. We can discuss if it should belong here or not when we introduce the actual code. If the comment is confusing, I can remove it. We decided to introduce the TODOs to give a better picture of the subsequent patches but if this is not helpful or annoying I can just get rid of all of them. Please, let me know what you think.
> I don't like the idea of outer-loops being in a special branch of the code, but I understand the current prototype nature of it. 
> 
> I believe it's still not the time to define what goes where that hasn't been implemented yet, so better to remove the TODOs for now, in case they lead us astray in the future. Same for the debug messages, etc.
> 
> What you should do is shortly explain why outer-loop needs "special handling", and that can be a one/two line comment in the beginning of the block.
> I believe it's still not the time to define what goes where that hasn't been implemented yet, so better to remove the TODOs for now, in case they lead us astray in the future.

> at you should do is shortly explain why outer-loop needs "special handling", and that can be a one/two line comment in the beginning of the block.

Agreed. Thanks!



https://reviews.llvm.org/D42447