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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D69069#1812947 <https://reviews.llvm.org/D69069#1812947>, @ekatz wrote:

> In D69069#1768576 <https://reviews.llvm.org/D69069#1768576>, @fhahn wrote:
>
> > Would it be possible to also add some of the test cases for the linked bugs?
>
>
> This patch fixes a crash. Maybe just add a .ll file that shouldn't crash (with the fix)?!


that would be ideal.



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


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:213
+    LI.erase(L);
     --NumLoops;
     ++NumExtracted;
----------------
ekatz wrote:
> 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.
Right.


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

https://reviews.llvm.org/D69069