[PATCH] D89566: [LV] Epilogue Vectorization with Optimal Control Flow

[Vinay Madhusudan via Phabricator via llvm-commits]

mivnay added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:175
+
+static cl::opt<bool> EnableEpilogueVectorization(
+    "enable-epilogue-vectorization", cl::init(false), cl::Hidden,
----------------
bmahjour wrote:
> xbolva00 wrote:
> > mivnay wrote:
> > > Why not enable it by default?
> > +1
> The reason I'm reluctant to enable it by default is because the heuristic for choosing when to vectorize the epilogue (and by what VF) is quite simplistic and tuned to a specific benchmark, without consideration for the cost of extra branches, code size increase, extra register spills, etc. 
> My thinking is we can move towards the goal of enabling it by default in smaller steps, by first implementing the transformation (this patch), then improving the cost-model along with performance tuning (future work). What do you think?
Unless you see some regressions, I think we can enable it by default.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:907
+  ElementCount EpilogueVF = ElementCount::getFixed(0);
+  unsigned EpilogueUF = 0;
+  BasicBlock *MainLoopIterationCountCheck = nullptr;
----------------
bmahjour wrote:
> mivnay wrote:
> > High UF might negate the benefit of EpilogVectorization. I think keeping `UF = 1` is a good idea unless there are multiple levels of epilog loop vectorization.
> I think so too, but I also thought it would be a good thing to make the UF configurable in case the need arises in the future (eg. with increasing vector widths and scalable vector types). In this patch, the only EpilogueLoopVectorizationInfo object is created with an epilogue UF of 1.
It would be good to assert `UF == 1` I guess.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89566