[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:
> > 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. 


================
Comment at: llvm/unittests/Transforms/Utils/LoopPeelTest.cpp:1
+//===- LoopPeelUtilsTest.cpp - Unit tests for LoopRotation utility ----===//
+//
----------------
reames wrote:
> Any reason this can't be a lit test?  Strongly preferred.
Currently, Loop peeling is not a pass in itself and the passes which use it in upstream requires LCSSA form or constructs it (LoopUnroll).

Now, that I mention it, I don't see LCSSA being a requirement in loop Fusion. So, maybe I can construct a testcase with fusion. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118472