[PATCH] D86844: [LoopDeletion] Allows deletion of possibly infinite side-effect free loops

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopDeletion.cpp:52
+  // can remove this loop.
+  if (hasMustProgress(L) || L->getHeader()->getParent()->mustProgress()) {
+    // Make sure that all PHI entries coming from the loop are loop invariant.
----------------
Early exists "always". Though I feel this is not the right place for this. Do you need it here at all when u check it also later?


================
Comment at: llvm/lib/Transforms/Scalar/LoopDeletion.cpp:215
+  if (isa<SCEVCouldNotCompute>(S) &&
+      !L->getHeader()->getParent()->mustProgress() && !hasMustProgress(L)) {
     LLVM_DEBUG(dbgs() << "Could not compute SCEV MaxBackedgeTakenCount.\n");
----------------
This seems to be the only change we need, isn't it?


================
Comment at: llvm/test/Analysis/ScalarEvolution/2012-03-26-LoadConstant.ll:1
-; RUN: opt < %s -basic-aa -globalopt -instcombine -loop-rotate -licm -instcombine -indvars -loop-deletion -constmerge -S | FileCheck %s
+; RUN: opt < %s -basic-aa -globalopt -instcombine -loop-rotate -licm -instcombine -indvars -loop-deletion -constmerge -simplifycfg -S | FileCheck %s
 ; PR11882: ComputeLoadConstantCompareExitLimit crash.
----------------
atmnpatel wrote:
> jdoerfert wrote:
> > Why?
> If we don't apply the pass, there are extra labels that are generated and left there i.e.
> ``   br label %step1
>    step1:
>      br label %step2
>    step2:
>    ...
> ``
> Does it make sense to remove that extra pass and check for this?
add/modify check lines as needed, I think running more passes here is not helpful, it runs enough as it is.


================
Comment at: llvm/test/Transforms/LoopDeletion/mustprogress.ll:85
+; CHECK-NEXT:    ret void
+;
+entry:
----------------
Why is this not deleted. We should know the trip count, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86844