[PATCH] D115713: [LV] Don't apply "TinyTripCountVectorThreshold" for loops with compile time known TC.

[Dave Green via Phabricator via llvm-commits]

dmgreen added a subscriber: SjoerdMeijer.
dmgreen added a comment.

I tried running the original benchmarks again with this patch series, it still sees the same decrease in performance I'm afraid. If it was a small change it would be understandable, we accidentally end up on the wrong side of the scalar cost and choose to vectorize where we shouldn't. But the new code is 40% slower than the scalar version, so it's quite a difference.  I haven't had a chance to look into the costs it produces, there is a chance we are underestimating the cost of predication or overestimating the cost of scalar. At worst, providing getMinTripCountTailFoldingThreshold works like it should we can always put a limit on the tripcount, providing we can find a decent minimum that works in general for MVE.

The vectorizer doesn't really do any cost modelling for the setup cost, past some obvious things like runtime checks. Due the the pass pipeline, it is usually expected that many low-trip-count loops are unrolled prior to vectorization, and we SLP them instead. D135971 <https://reviews.llvm.org/D135971> looks like it changes a lot of code in an area that has caused an amount of trouble in the past. We know that the Vectorizer current needs to commit to tail folding vs not tail folding too early. It would be better to have that option as part of the vplan, allowing predicated and non-predicated patters to be costed against one another. I'm not sure I follow D135971 <https://reviews.llvm.org/D135971> enough to understand the ramifications of those changes.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9870-9875
+      // If trip count is known compile time constant there is no any extra
+      // overhead from vectorization with scalar epilogue. Let cost model to
+      // decide on profitability in that case.
+      auto TC = SE.getSmallConstantTripCount(L);
+      if (!TC) {
+        if (*ExpectedTC <= TTI->getMinTripCountTailFoldingThreshold()) {
----------------
ebrevnov wrote:
> paulwalker-arm wrote:
> > I don't quite understand this change.  The whole point of `getMinTripCountTailFoldingThreshold()` was to give targets control over this behaviour based on their understanding how the cost model has been implemented.
> > 
> > Admittedly this was in part due to the immaturity of the cost modelling but this change essentially removes that flexibility to the point where there's no value in keeping `getMinTripCountTailFoldingThreshold()`?
> > 
> > If your previous patches improve the cost model in this regard then I'd rather `getMinTripCountTailFoldingThreshold()` be removed.  That said, @dtemirbulatov can you help here in ascertaining if this option is still required based on this new patch series?
> I think you are missing one thing. Currently we allow vectorization of short trip count loops if there is  no run-time overhead and the tail can be folded only. Introduction of getMinTripCountTailFoldingThreshold put additional limitations on vectorization of such loops. This change enables vectorization of short trip count loops with scalar epilogue ONLY If trip count is compile time constant. getMinTripCountTailFoldingThreshold is still applicable if we decide to vectorize by folding the tail.
> 
> In other words, this change enables exactly the same way of vectorization for loops with small constant trip count as for loops with unknown or large trip count.
> 
> It's a question whether getMinTripCountTailFoldingThreshold threshold should be taken into account if it's decided to fold the tail at step 5) for short trip count loop. Possibly yes...  I think this is your original concern, right?
Are you assuming that the loop will be unrolled after vectorization, and that unrolling will leave no extra overhead? Scalable vector loops cannot (currently) be unrolled.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115713/new/

https://reviews.llvm.org/D115713