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

[Ehud Katz via Phabricator via llvm-commits]

ekatz marked an inline comment as done.
ekatz added inline comments.


================
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();
+
----------------
ekatz wrote:
> fhahn wrote:
> > ekatz wrote:
> > > 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.
> > I think the position of my original comment got messed up. I was referring to the distinction between functions with a single loop and functions with multiple loops. I *think* this was only needed with the LoopPass to avoid extracting loops we already extracted into new functions earlier which might result in an infinite loop.
> > 
> > I.e. just have the following starting at line 140.
> > 
> > ```  
> > DominatorTree &DT = getAnalysis<DominatorTreeWrapperPass>(F).getDomTree();
> > return extractLoops(LI.begin(), LI.end(), LI, DT);
> > ```
> > 
> > The test coverage isn't great, but it seems to pass all tests ;)
> Not sure the position got messed up - it is probably my fault. I do not sleep much these days, with my 1 year old teething or whatever he decides the reason to wake up every  hour at night...
> 
> Anyway, this code is from SVN commit rL12403 (Git commit rG2f155d87). It is from the time that the pass was a FunctionPass. It tries to distinguish between a function that has more than a single loop with a simple function wrapper around a loop (which can be created by other means than the extractor).
I'll add a test for that particular case.


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

https://reviews.llvm.org/D69069