[PATCH] D70939: [LoopUtils] Updated deleteDeadLoop() to handle loop nest.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:676
     // The last step is to update LoopInfo now that we've eliminated this loop.
-    LI->erase(L);
+    std::function<void(Loop *)> EraseLoopNest = [&](Loop *L) {
+      for (Loop *SubLoop : *L)
----------------
[remark] Assigning a lambda to a `std::function` introduces a lot of overhead. However, `auto` doesn't work for calling itself recursively. https://laptrinhx.com/recursive-lambdas-713741980/ has a nice technique though.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:677
+    std::function<void(Loop *)> EraseLoopNest = [&](Loop *L) {
+      for (Loop *SubLoop : *L)
+        EraseLoopNest(SubLoop);
----------------
[serious] `LoopInfo::erase` is designed to modify the parent's subloop list. At least, it will remove `Subloop` from `L`'s subloop list, hence invalidating the iterator we are currently iterating over.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:681
+      assert(L->getSubLoops().empty() && "Expecting to be innermost loop");
+      LI->erase(L);
+    };
----------------
[suggestion] Could you add a comment such as "Recursively remove all subloops. Calling LoopInfo::erase only on the dead loop only moves its subloops to the dead loop's parent".

`LoopInfo::erase` does a loot of work to move the association of the subloop blocks to the parent. Since we know that the loop is dead, I wonder whether it is worth introducing a variant that just removes the dead loop from its parent's child list.


================
Comment at: llvm/unittests/Transforms/Utils/LoopUtilsTest.cpp:28
+                    Test) {
+  auto *F = M.getFunction(FuncName);
+  DominatorTree DT(*F);
----------------
[style] No almost always auto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70939