[PATCH] D30247: Epilog loop vectorization

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Hi Ashutosh,

Sorry for the delay. I think the patch is looking good from my end.

I'd add a `TODO` to maybe unroll+SLP if the proportion between the next profitable VF and the size of the tail are close together. This could also simplify the call graph.

Example: VF=8, so tail is at most 7 and next VF is 4. This would be better without a latch, just `if (end - i) > 4` -> 4-way SIMD -> `for (i .. end)` -> scalar.

But this is not a job for this patch, I think.

So, now, it would be good to know a few numbers. Can you share some performance improvements?

Also, would be good to get @mkuper take on this.

cheers,
--renato



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3280
+  // Should be legal to vectorize.
+  if (!LVL.canVectorize()) {
+    DEBUG(dbgs() << "MVer-LV: Not vectorizing: Cannot prove legality.\n");
----------------
ashutosh.nema wrote:
> We can remove call to loop vectorizer’s legality before widening epilog loop, as legality is already proven for scalar loop while generating first vector version, it’s not required to prove again as scalar body has not changed. Epilog vectorization will depend on already computed legality. I have tried this change and found no impacts in my regular tests & benchmarks.
> 
> Please let me know thoughts on this.
> 
Legality is not the problem, here, but the cost. If the transformation is legal for `VF`, then it's also legal for all `vf`s < `VF`.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6420
+LoopVectorizationCostModel::VectorizationFactor
+LoopVectorizationCostModel::identifyNextProfitableVF(const unsigned Width,
+                                                     bool OptForSize) {
----------------
This makes sense to me, and it's exactly how I would implement it.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6430
+    return VectorizationFactor(VF, Cost);
+  // NOTE: Currently multiple induction variables not supported as multiple
+  // induction variable incurs multiple checks which may not be profitable.
----------------
You don't need the `NOTE` in comments...


Repository:
  rL LLVM

https://reviews.llvm.org/D30247