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

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.

LGTM, with comments addressed before committing. Thanks!



================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:151
+
+  // If LoopSimplify form is not available, stay out of trouble.
+  if (TLL->isLoopSimplifyForm()) {
----------------
nit: the comment is a bit confusing, as it checks if the loop is in simplify-form. Maybe say something like: If the loop is in LoopSimplify form, check if the function is a minimal container around the loop.


================
Comment at: llvm/test/Transforms/CodeExtractor/LoopExtractor.ll:3
+
+; CHECK: define
+; new 2 outlined function
----------------
I think it would be good to check the full IR after extraction.


================
Comment at: llvm/test/Transforms/CodeExtractor/LoopExtractor_crash.ll:4
+
+; This test used to trigger an assert.
+
----------------
There are no arguments to promote and no functions to inline? Is -inline/-argpromotion needed? Also, please add some rudimentary checks. (you could use llvm/tools/update_test_checks.py)


================
Comment at: llvm/test/Transforms/CodeExtractor/LoopExtractor_infinite.ll:3
+
+; This test used to enter an infinite loop (until out of memory).
+
----------------
Is running -mergereturn required? Or is it enough to have the output of merge return as test case? Also it would be good to have some rudimentary checks.


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

https://reviews.llvm.org/D69069