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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
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?");
 
----------------
reames wrote:
> 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. :)
DT is definitely non-null and we have an (unintentional) assert for this at line 915 already :). This makes the code cleanup here simpler. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118472