[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();
+
----------------
fhahn wrote:
> ekatz wrote:
> > 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.
> > 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).
> 
> Right, but it cannot happen while the LoopExtractor is running, with it being a module pass now, so we cannot get stuck in a loop where we extract a  loop into a wrapper function and then try to extract the loop from the newly created function and get stuck.
> 
> Of course it still might be slightly more preferable to avoid extracting loops from trivial wrapper functions, but IMO the extra complexity does not really warrant the smallish benefit on a few probably not very common cases (also the checks are quite brittle and I guess not hit very often in practice (unless running loop-extract multiple times...))
> 
> But I am not feeling very strongly about that point :)
The new test LoopExtractor_min_wrapper.ll demonstrates the use case.


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

https://reviews.llvm.org/D69069