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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:185
+
+static cl::opt<unsigned> EpilogueVectorizationMinVF(
+    "epilogue-vectorization-minimum-VF", cl::init(16), cl::Hidden,
----------------
Think we need a test-case for it too, perhaps one that is not the default value 16.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5659
+
+  // Phis with uses outside of the loop require special handling and are
+  // currently unsupported.
----------------
I was hoping and seemed to remember there was a LoopUtils helper for this. Had a quick look, perhaps `findDefsUsedOutsideOfLoop` could be useful, or there was another function here in the vectoriser that was doing the same/similar things for reductions. Perhaps have a look if there's something we can reuse.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5730
+
+  if (EpilogueVectorizationForceVF > 1) {
+    LLVM_DEBUG(
----------------
I am wondering which option should win: e.g. `-Os` or `EpilogueVectorizationForceVF`? I am guessing that is the forced epilogue vectorisation? Should the opt size check be moved? 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5732
+    LLVM_DEBUG(
+        { dbgs() << "LEV: Epilogue vectorization factor is forced.\n"; });
+    if (LVP.hasPlanWithVFs(
----------------
nit: don't think we need the curly brackets here in LLVM_DEBUG. 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5737
+    else {
+      LLVM_DEBUG({
+        dbgs() << "LEV: Epilogue vectorization forced factor is not viable.\n";
----------------
same here


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5753
+
+  if (!(Result == VectorizationFactor::Disabled()))
+    LLVM_DEBUG({
----------------
nit: `!=` ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5754
+  if (!(Result == VectorizationFactor::Disabled()))
+    LLVM_DEBUG({
+      dbgs() << "LEV: Vectorizing epilogue loop with VF = "
----------------
nit: curly bracket


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9152
+
+      simplifyLoop(L, DT, LI, SE, AC, nullptr, false);
+      formLCSSARecursively(*L, *DT, LI, SE);
----------------
`false` is for `PreserveLCSSA`, but do we want that to be true, and does that make  formLCSSARecursively redundant? Not sure if I am suprised that this is necessary at all....

nit: I think it should be at least `false /* PreserveLCSSA */`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89566