[PATCH] D69069: [IPO] Convert LoopExtractor pass from LoopPass to ModulePass

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Some comments, but it looks reasonable overall.



================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:124
+
+bool LoopExtractor::visitLoop(Loop *L, DominatorTree &DT, LoopInfo &LI) {
   // If LoopSimplify form is not available, stay out of trouble.
----------------
Nit: rename to `runOnFunction` for consistency? I don't mean having an override method.
To give a similar example, function passes may have a static or class specific `runOnBasicBlock` without being a basic block pass.


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:41
     static char ID; // Pass identification, replacement for typeid
     unsigned NumLoops;
 
----------------
Could you add a comment on how `NumLoops` is used?


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:103
   bool Changed = false;
+  SmallVector<Loop *, 8> TopLevelLoops;
+
----------------
Unused after the latest change?


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:160
+    TLL->getExitBlocks(ExitBlocks);
     for (unsigned i = 0, e = ExitBlocks.size(); i != e; ++i)
       if (!isa<ReturnInst>(ExitBlocks[i]->getTerminator())) {
----------------
`for (auto *ExitBlock : ExitBlocks)`


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:202
+  L->getExitBlocks(ExitBlocks);
+  for (unsigned i = 0, e = ExitBlocks.size(); i != e; ++i)
+    if (ExitBlocks[i]->isEHPad())
----------------
`for (auto *ExitBlock : ExitBlocks)`


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

https://reviews.llvm.org/D69069