[PATCH] D118472: [LoopPeel] Check for non-LCSSA form loops

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Basic idea looks reasonable, though we appear to have bit of cleanup to do before this can reasonably land.



================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:195
+        GuardBranch(L->getLoopGuardBranch()), PP(PP),
+        AbleToPeel(canPeel(L, *(const_cast<DominatorTree *>(DT)), *LI)),
+        Peeled(false), DT(DT), PDT(PDT), LI(LI), ORE(ORE) {
----------------
mkazantsev wrote:
> Loop peeling pass is not guaranteed to be provided with `DT` and `LI`. They can both be (and sometimes are) nullptr, at least in old PM.
Max, your comment may be correct for LoopPeeling (haven't checked yet), but the LoopFuze code does appear to require DT and LI today.  As such, they can't be null along this particular codepath.  


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:748
   assert(PeelCount > 0 && "Attempt to peel out zero iterations?");
-  assert(canPeel(L) && "Attempt to peel a loop which is not peelable?");
+  assert(canPeel(L, *DT, *LI) && "Attempt to peel a loop which is not peelable?");
 
----------------
mkazantsev wrote:
> This should also work for `DT == nullptr`, see checks below (line 764).
Chasing through the code, I think the handling for nullptr DT is simply dead.  It definitely used to be supported, but it looks like the callers (both fuse and unroll) now require DT.  

I also see some code which looks like it would crash if a null DT was actually passed in.

Anna, could you do a cleanup which folded the DT null checks and asserted it was never null?  I think we need that before we can land this one.


================
Comment at: llvm/unittests/Transforms/Utils/LoopPeelTest.cpp:1
+//===- LoopPeelUtilsTest.cpp - Unit tests for LoopRotation utility ----===//
+//
----------------
Any reason this can't be a lit test?  Strongly preferred.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118472