[PATCH] D23148: [LoopUnroll] Simplify loops created by unrolling.

[Michael Zolotukhin via llvm-commits]

mzolotukhin added inline comments.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:386-387
@@ +385,4 @@
+  SmallSetVector<Loop *, 4> LoopsToSimplify;
+  for (Loop *SubLoop : *L)
+    LoopsToSimplify.insert(SubLoop);
+
----------------
chandlerc wrote:
> So, this is done because we leave the first copy of the loop alone when unrolling?
> 
> If not, shouldn't we handle this below so that we don't add inner loops that won't actually exist after unrolling?
> this is done because we leave the first copy of the loop alone when unrolling?
Yes, exactly. When we unroll something like this:
```
outer_loop {
  inner_loop
}
```
we could get something like this:
```
outer_loop {
  inner_loop
  inner_loop2
}
```

or even
```
  inner_loop
  inner_loop2
```
in the case of complete unrolling.

`inner_loop2` is a new loop, created by loop-unrolling (see lines 417-418). `inner_loop` existed in the original IR and remains (almost) untouched. It still needs to be simplified, as it'll be using the same exit block, as the cloned loops.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:692
@@ +691,3 @@
+    } else {
+      // Simplify loops for which we might've broken loop-simplify form.
+      for (Loop *SubLoop : LoopsToSimplify)
----------------
chandlerc wrote:
> Related to the above TODO comment, the ways in which this can break loop simplified form seem fairly constrained. If this is a compile time issue, its likely a more targeted approach could be used... Happy to just leave a comment here if you don't yet know whether we need to consider the compile time here.
> the ways in which this can break loop simplified form seem fairly constrained
This is true, that's why I left the TODO there. It should be definitely possible to fix only broken parts (i.e. insert preheaders and split exit-edges). On the other hand, we've been calling `simplifyLoop` on `OuterL` without any issues before, which also simplifies all nested loops - that was my motivation for keeping it simple for now. My idea was that comment from above relates to this spot as well, just didn't want to replicate the same TODO.


https://reviews.llvm.org/D23148