[PATCH] D19553: Unroll pass restructure.

[Evgeny Stupachenko via llvm-commits]

evstupac added a comment.

Thanks for taking a look on the patch.
Please find my answers inline.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:539
@@ +538,3 @@
+    UP.Force = true;
+    if ((LoopSize - 2) * UP.Count + 2 < UP.Threshold
+        && UP.AllowRemainder)
----------------
zzheng wrote:
> This formula appears several times... It'll be better to have a consolidated function that computes the unrolled size or enforces the threshold.
Good point.
We can create something like IsCountMeetThreshold(UP, LoopSize)

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:564
@@ -575,1 +563,3 @@
+  bool HasPragma = PragmaCount > 0 || PragmaFullUnroll || PragmaEnableUnroll ||
+                   UnrollCount.getNumOccurrences() > 0;
 
----------------
zzheng wrote:
> This feels a little awkward. I would use
> 
> ```
> bool UserUnrollCount = UnrollCount.getNumOccurrences() > 0;
> ...
> 
> if (HasPragma || UserUnrollCount)
> ```
> 
Agree. This is better.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:305
@@ +304,3 @@
+    if (Force)
+      RuntimeTripCount = false;
+    else
----------------
zzheng wrote:
> I don't understand this part. If UnrollRuntimeLoopRemainder() returned false, remainder loop is not generated. How do we ensure correctness if we 'force' a loop that has runtime tripcount of 6 to by unrolled by 4?
Actually that is the way how -unroll-count works now when runtime unroll is disabled.
For this type of unrolling we do not remove conditional branches from unrolled loop. For example unroll by 2:

for.body
  ...
  cmp
  br for.body1, exit
for.body1
  ...
  cmp
  br for.body, exit
exit

My changes do not change current behavior.
In my next patch I'll request runtime unroll to be enabled when -unroll-count passed.

There could be positive effects of this type of unrolling (like reduced number of executed backward branches). So if user wants a loop to be unrolled compiler should do this (when it is safe).



Repository:
  rL LLVM

http://reviews.llvm.org/D19553