[PATCH] D79783: [LV] Fallback strategies if tail-folding fails

[Dave Green via Phabricator via llvm-commits]

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

If you do something about the spelling of epilog, then this looks good to me.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5105
+  if (ScalarEpilogueStatus == CM_ScalarEpilogueNotNeededUsePredicate) {
+    if (PreferPredicateOverEpilogue == PreferPredicateTy::PredicateOrDontVectorize) {
+      LLVM_DEBUG(dbgs() << "LV: Can't fold tail by masking: don't vectorize\n");
----------------
SjoerdMeijer wrote:
> dmgreen wrote:
> > So if I understand this logic properly, the loop hint or the targethook now both mean "try tailpredicate but fall back to vectorize if you can't". The PreferPredicateOverEpilogue option can mean "only tail predicate" or "try tailpredicate, fallback if needed" depending on the choice of the option? (I had not originally realized that was already how this worked for the target hook.)
> > 
> > Do we want the pragma and targethook to always work like that? Or is it worth giving the target hook a choice between the two options. I suspect for MVE we would always want to choose the "with fallback" option, so maybe it's not worth adding?
> > 
> > If we did, we could possibly add another ScalarEpilogueStatus value, to not have to recheck PreferPredicateOverEpilogue or the target hook here.
> > So if I understand this logic properly, the loop hint or the targethook now both mean "try tailpredicate but fall back to vectorize if you can't". The PreferPredicateOverEpilogue option can mean "only tail predicate" or "try tailpredicate, fallback if needed" depending on the choice of the option? (I had not originally realized that was already how this worked for the target hook.)
> 
> Yep, that is exactly right I think.
> 
> > Do we want the pragma and targethook to always work like that? Or is it worth giving the target hook a choice between the two options. I suspect for MVE we would always want to choose the "with fallback" option, so maybe it's not worth adding?
> 
> My first and perhaps naive thoughts were that we always want to vectorise, so set that as the fallback, because I thought this was guaranteed to be a win. But then some benchmarks proved me wrong. From a first look, however, this is just exposing some inefficiencies elsewhere. So, I still think it is the right choice to make in the end. 
> 
> The way I look at this patch is that this is an enabler, and if we want to flip the switch by default somehow, then that's probably best done as a follow up of this groundwork. 
I think this probably does what we want for MVE, and seems like a sensible default.


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

https://reviews.llvm.org/D79783