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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Would it be possible to also add some of the test cases for the linked bugs?



================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:144
+  // the loops.
+  if (std::distance(LI.begin(), LI.end()) > 1)
+    return extractLoops(LI.begin(), LI.end(), LI, DT);
----------------
It might be slightly better to use size (from STLExtra.h), as it will fail to compile, if the iterators are not random access iterators. Or just check `std::next(LI.begin()) != std::end()`


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:149
+  // function is more than a minimal wrapper around the loop, extract the loop.
+  Loop *TLL = *LI.begin();
+
----------------
Do we actually need this special case when it's a module pass? IIUC, it was only needed with the LoopPass to avoid extracting loops we already extracted into new functions earlier right?


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:181
+  // sub-loops, extract them.
+  return extractLoops(TLL->begin(), TLL->end(), LI, DT);
+}
----------------
I am not sure I see why we would try to extract the sub loops here? The original loopPass only tried to extract top-level loops, unless I missed something.


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:213
+    LI.erase(L);
     --NumLoops;
     ++NumExtracted;
----------------
The original NumLoops was on a per-function basis I guess, but now it's per-module, right? I think it would be better to keep it on a per-function basis, i.e. NumLoops == the max number of loops to extract per function. 


================
Comment at: llvm/test/Feature/optnone-opt.ll:57
 ; OPT-LOOP-DAG: Skipping pass 'Delete dead loops'
-; OPT-LOOP-DAG: Skipping pass 'Extract loops into new functions'
 ; OPT-LOOP-DAG: Skipping pass 'Induction Variable Simplification'
----------------
The pass is still run right, just somewhere else? Could you check this in the right place?


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

https://reviews.llvm.org/D69069