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

[Ehud Katz via Phabricator via llvm-commits]

ekatz marked 5 inline comments as done.
ekatz added inline comments.


================
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);
----------------
fhahn wrote:
> 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()`
I agree. `size` (from STLExtras.h) is better.
Regarding the `std::next` solution, I thought it is a bit less clear, but it is actually even a better solution for the general case. So, I'll change it to that.


================
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();
+
----------------
fhahn wrote:
> 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?
This particular line was added in SVN commit r86175 (Git commit rGa83ac2d9e7b990df6978598d9dec860b5ef13a9a). Personally, I do not see a good reason not to extract loops in a non simplify-form, but the change (that added it) does not explain much, so I decided to keep it (maybe there was a related bug?).
If you are positive that it is not needed, then I'll remove it.


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:181
+  // sub-loops, extract them.
+  return extractLoops(TLL->begin(), TLL->end(), LI, DT);
+}
----------------
fhahn wrote:
> 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.
As described in the "review summary", this is a kind of revert of SVN commit r82990 (Git commit rG9a7320c7110ee7f5450f8c46abe361de769886c0). I don't know why the extraction of the sub-loops was removed in that change, other than, probably, the change to LoopPass. Seems like an unintentional change/bug.


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:213
+    LI.erase(L);
     --NumLoops;
     ++NumExtracted;
----------------
fhahn wrote:
> 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. 
It used to be per program, as it is now; as the previous LoopPass was created only once per program (as well as the new ModulePass). The result remains the same.


================
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'
----------------
fhahn wrote:
> The pass is still run right, just somewhere else? Could you check this in the right place?
It is still run, but it does not have an indication that the pass is skipped. Not sure why it was designed that way (probably wasn't), but skipped ModulePasses (via a call to `skipModule`) do not print any indication, while Function/Loop/Region-passes do.


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

https://reviews.llvm.org/D69069