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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
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:
> reames wrote:
> > 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.  
> Then the first step would be to replace `DominatorTree *` with `DominatorTree &` to be adamant. Now this fact is at least not obvious.
The first step is to add asserts, run tests, and check them in if nothing fails.  The *second* step is to convert the assert into a refactoring change to use references where possible.  


================
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:
> anna wrote:
> > reames wrote:
> > > 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.
> > Yes, I'll do the cleanup. Note that the only use case of concern maybe downstream users with peeling without DT? But we definitely need DT to make this change. 
> I vaguely remember that we've seen some real cases of null DT while working on some peeling patch. Maybe it only happens in unit tests, though. I don't mind if we rework this code so that DT is always non-null, but let's clearly state it (and replace pointer with reference).
I vaguely remember this too.  That's why I want asserts to find our counter example if one exists. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118472