[PATCH] D94892: [LV] Unconditionally branch from middle to scalar preheader if the scalar loop must execute

[Florian Hahn via Phabricator via llvm-commits]

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

In D94892#2534445 <https://reviews.llvm.org/D94892#2534445>, @reames wrote:

> Florian, have you had a chance to give thought to alternatives?  Given it's been two weeks, unless you have an actionable suggestion, I'd like to move forward with the current patch.  I'll emphasize that I'm happy to iterate on the design here, either during review, or after submission if you think of something cleaner.

Unfortunately I did not really have time to think much about better alternatives so far (and it's unlikely I'll get time until end of next week).  But I agree, it's not a really big deal and it works well enough for now. Let's tweak it post-commit, should a better alternative presents itself. So for now I just have a few small suggestions, mostly wording related.

LGTM, thanks!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3109
+  if (!Cost->requiresScalarEpilogue())
+    // For loops with multiple exits, there's no edge from the middle block
+    // to exit blocks (as the epilogue must run) and thus no need to update
----------------
This applies for all loops that require scalar epilogues, not just ones with multiple exits, right? If so, it might be better word it in terms of requiring scalar epiloge, rather than multi exits. (perhaps something like `An edge from the middle block to the exit block is only added if the scalar epilogue may not be executed. Thus only update the immediate dominators if the scalar epilogue is not required`.)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3357
+  // 2) Otherwise, we must have a single unique exit block (due to how we
+  //    implement the multiple exit case).  In this case, set up conditonal
+  //    branch from middle block to loop scalar preheader, and the exit block.
----------------
nit: set up `a` conditional branch from `the` middle block to the scalar loop pre-header?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3365
+    }
+    assert(LoopExitBlock && "Must have an exit block");
+    return BranchInst::Create(LoopExitBlock, LoopScalarPreHeader,
----------------
Do we need this assert? There's a similar assert just a few lines above. If it's not needed, getting rid of the lambda would make things a bit easier to read IMO (perhaps it could be just `BranchInst *BrInst = Cost->requiresScalarEpilogue() ? BranchInst::Create(LoopScalarPreHeader) : BranchInst::Create(LoopExitBlock, LoopScalarPreHeader, Builder.getTrue())`)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3483
+  // 1) If we require scalar epilogue, there is no conditional branch as
+  //    we unconditionally branch to scalar preheader.  Do nothing.
+  // 2) If (N - N%VF) == N, then we *don't* need to run the remainder.
----------------
nit: if we require *a* scalar epilogue .... as we unconditionally branch to *the* scalar preheader?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3626
                                        BasicBlock *MiddleBlock) {
+
   // There are two kinds of external IV usages - those that use the value
----------------
nit: unrelated whitespace change


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

https://reviews.llvm.org/D94892