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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour 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,
----------------
SjoerdMeijer wrote:
> Think we need a test-case for it too, perhaps one that is not the default value 16.
test added


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5659
+
+  // Phis with uses outside of the loop require special handling and are
+  // currently unsupported.
----------------
SjoerdMeijer wrote:
> 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.
I was trying to match the conditions in `fixupIVUsers()` and added early exits to make sure we don't waste compile-time collecting information that will be thrown away. Maybe we can add a flag to `findDefsUsedOutsideOfLoop` to stop collecting instructions as soon as one is found, but then we'll have a new problem in that the checks in this code would be different from what's in `fixupIVUsers()`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5730
+
+  if (EpilogueVectorizationForceVF > 1) {
+    LLVM_DEBUG(
----------------
SjoerdMeijer wrote:
> 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? 
I can see arguments for it either way, but after thinking more about it, I believe it makes it easier to reason about the behaviour of the loop-vectorizer when the forced vf wins over the minsize. I'll change it.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5732
+    LLVM_DEBUG(
+        { dbgs() << "LEV: Epilogue vectorization factor is forced.\n"; });
+    if (LVP.hasPlanWithVFs(
----------------
SjoerdMeijer wrote:
> nit: don't think we need the curly brackets here in LLVM_DEBUG. 
I have developed a habit of adding them because I find it makes the enclosed code more friendly to clang-format. I'll remove the brackets here since they don't make much of a difference to the formatting in this case.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5753
+
+  if (!(Result == VectorizationFactor::Disabled()))
+    LLVM_DEBUG({
----------------
SjoerdMeijer wrote:
> nit: `!=` ?
Currently `VectorizationFactor` only overrides `operator==` not the `operator!=`. I'll provide an override and change this to `!=`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9152
+
+      simplifyLoop(L, DT, LI, SE, AC, nullptr, false);
+      formLCSSARecursively(*L, *DT, LI, SE);
----------------
SjoerdMeijer wrote:
> `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 */`
I think we could get away without calling `formLCSSARecursively` for now and set `PreserveLCSSA` to true for the limited cases of live-out values supported (see newly added `Transforms/LoopVectorize/PowerPC/optimal-epilog-vectorization.liveout.ll`). However in the future if we add support for more live-outs (eg. reductions and first-order recs), I worry that we may be in a state at this point in the code where the original loop is temporarily non-LCSSA which would break `simplifyLoop`'s assumptions if we set `PreserveLCSSA` to true. Note that when `PreserveLCSSA` is true, `simplifyLoop` assumes that the loop is already in LCSSA form.
Maybe I worry too much about it, but I think the current way is a bit more future-proof. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89566