[PATCH] D18290: Unroll of loops with constant bounds

[Evgeny Stupachenko via llvm-commits]

evstupac added a comment.

Hi Michael,

Thanks for the review.
Please find my responses inline.

Thanks,
Evgeny


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:646-651
@@ +645,8 @@
+        // satisfies the threshold limit.
+        Count = (std::max(UP.PartialThreshold, 3u)-2) / (LoopSize-2);
+        UnrolledSize = (LoopSize - 2) * Count + 2;
+        while (Count != 0 && UnrolledSize > UP.PartialThreshold) {
+          Count >>= 1;
+          UnrolledSize = (LoopSize - 2) * Count + 2;
+        }
+      }
----------------
mzolotukhin wrote:
> Do I understand it correctly that with this change we start to unroll every loop if it's possible at all (some of them with remainder)? If that's correct, have you measured performance, compile time, and code size impact of this change? Also, it might make sense to limit it to O3.
The change allows unroll only for loops that satisfy threshold limit. There are not much cases where this hits. On spec2000 I've got almost all build same or <0.1% code size changes.
Without the changes it happens that the same loop with unknown bounds get unrolled, but with constant no.
For example when we have prime TripCount we'll not found Count that is modulo of TripCount.

```
for (i = 0; i < 17; i++)
```
I don't see any reason why we should restrict unroll in the case if we are in threshold limits.

As for remainder - there is no such by default. As we know remainder TripCount (it is constant) we can jump into the middle of the loop first.

The other point is that now we check threshold limit only at entrance. So potentially we can find unroll factor which exceed threshold limit.


================
Comment at: test/Transforms/LoopUnroll/partial-unroll-const-bounds.ll:1
@@ +1,2 @@
+; RUN: opt < %s -S -unroll-threshold=20 -loop-unroll -unroll-allow-partial -O2 | FileCheck %s
+
----------------
mzolotukhin wrote:
> You probably don't want to run `opt ... -O2` in this test. O2 will run the entire optimization pipeline, while we only want loop-unroll.
"-O2" makes CHECK statements easier. However I agree it is not required here. I'll fix the test.

================
Comment at: test/Transforms/LoopUnroll/partial-unroll-const-bounds.ll:19
@@ +18,3 @@
+  %arrayidx = getelementptr inbounds i32, i32* %b, i64 %indvars.iv
+  %0 = load i32, i32* %arrayidx, align 4
+  %idxprom1 = sext i32 %0 to i64
----------------
mzolotukhin wrote:
> Please pass the test through `opt -instnamer` to replace names like %0 with some symbolic names. These digit names make it harder to change the test in future.
> 
> Also, you probably could only remove almost all instructions from the body to make the test smaller. If you run only loop-unroll, nothing will optimize that to `ret void`, so it would be fine.
Ok. Will update.


Repository:
  rL LLVM

http://reviews.llvm.org/D18290