[PATCH] D14526: Don't recompute LCSSA after loop-unrolling when possible.

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM as soon as you have a test case for the top-most loop!


================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:227
@@ +226,3 @@
+  Loop *ParentL = L->getParentLoop();
+  bool AllExitsAreInsideParentLoop = !ParentL ||
+      std::all_of(ExitBlocks.begin(), ExitBlocks.end(),
----------------
mzolotukhin wrote:
> joker.eph wrote:
> > It seems you didn't update the test, if it was passing before it means this is not tested in the non-regression suite?
> I didn't wait for test-suite when I made this change as I erroneously considered it NFC. However, with it even compiler-rt failed to build, so we definitely have tests for this case.
I still think we should have a regression test that exercises this situation.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:563-567
@@ -556,3 +562,7 @@
 
-      formLCSSARecursively(*OuterL, *DT, LI, SE);
+      if (CompletelyUnroll && !AllExitsAreInsideParentLoop)
+        formLCSSARecursively(*OuterL, *DT, LI, SE);
+      else
+        assert(OuterL->isLCSSAForm(*DT) &&
+               "Loops should be in LCSSA form after loop-unroll.");
     }
----------------
I agree with your reasoning, and the assert is a good way to stay confident that we don't break this going forward.

================
Comment at: test/Transforms/LoopUnroll/rebuild_lcssa.ll:19
@@ +18,3 @@
+; CHECK-LABEL: @foo1
+; Function Attrs: nounwind ssp uwtable
+define void @foo1() {
----------------
All of the function attrs comments seem stale (the attrs are gone).

================
Comment at: test/Transforms/LoopUnroll/rebuild_lcssa.ll:24
@@ +23,3 @@
+
+L1_header:                                        ; preds = %L1_latch, %entry
+  br label %L2_header
----------------
I'd really prefer to strip all the predecessor comments. It makes it a lot harder to update IMO.

================
Comment at: test/Transforms/LoopUnroll/rebuild_lcssa.ll:78
@@ +77,3 @@
+; CHECK:      L3_break_to_L1:
+; CHECK-NEXT:   lcssa = phi i64 [
+L3_break_to_L1:
----------------
It'd be good to check the structure here (ie, that its a single-entry phi from the exiting block).


http://reviews.llvm.org/D14526