[PATCH] D18290: Unroll of loops with constant bounds

Michael Zolotukhin via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 21 14:51:20 PDT 2016


mzolotukhin added a comment.

Hi Evgeny,

Please find some comments inline.

Thanks,
Michael


================
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;
+        }
+      }
----------------
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.

================
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
+
----------------
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.

================
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
----------------
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D18290





More information about the llvm-commits mailing list