[PATCH] D19553: Unroll pass restructure.

[Evgeny Stupachenko via llvm-commits]

evstupac added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:537-544
@@ -549,1 +536,10 @@
+  bool UserUnrollCount = UnrollCount.getNumOccurrences() > 0;
+  if (UserUnrollCount) {
+    UP.Count = UnrollCount;
+    UP.AllowExpensiveTripCount = true;
+    UP.Force = true;
+    if (UP.AllowRemainder &&
+        (LoopSize - 2) * UP.Count + 2 < UP.Threshold)
+      return true;
+  }
 
----------------
mzolotukhin wrote:
> I find it a bit confusing that we change values in `UP` even if we don't exit the function. Can we rewrite it to something like:
> ```
> If (condition1) {
>   UP = ...;
>   return true; // We only change UP when we are going to return
> }
> if (condition2) {
>   UP = ...;
>   return true;
> }
> ```
> as opposed to
> ```
> if (condition1) {
>   UP = ...
>   if (condition)
>     return true;
>   // Here we changed UP, but didn't return
> }
> ```
We can do so, but still we need to enable all the properties somehow.
For "condition1" it will look like:

  If (condition1) {
    UP = ...;
    return true; // We only change UP when we are going to return
  }
  if (condition1)
    UP = ...;

That is because if user specified count that exceed threshold we are reducing it, but still allow some certain level of unrolling (Runtime, Full, Force....) 

Personally I'd better disable unroll and warn user in that case (telling how to enable specified unroll by increasing pragma threshold or reducing count). That will resolve the issue, but change current behavior:

  if (condition1) {
    if (condition) {
      UP = ...
    } else {
      UP.Count = 1;
      Warn();
    }
    return true;
  }

Now it is very annoying when I tell compiler to unroll by 8, but it unrolls by 4 just because of some heuristics without any warnings.



Repository:
  rL LLVM

http://reviews.llvm.org/D19553