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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

This LGTM, pending the test cases.



================
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:
> 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 :)


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

https://reviews.llvm.org/D69069