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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Thanks for looking Dave.
I am addressing your remarks, also find some replies inline.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:194-195
+
+static cl::opt<PreferPredicateTy::Option> PreferPredicateOverEpilogue(
+    "prefer-predicate-over-epilog",
+    cl::init(PreferPredicateTy::ScalarEpilogue),
----------------
dmgreen wrote:
> We should probably pick a single spelling of epilog and stick to it. At least in the same option!
> Epilog is nice because it's shorter, but Epilogue seems to be the more common choice?
> 
> But feel free to ignore too if you like.
No, good point!

I honestly don't know what it should be and have no opinion on it, but I will go for the common case epilogue.


================
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");
----------------
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. 


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

https://reviews.llvm.org/D79783