[PATCH] D26989: Use continuous boosting factor for complete unroll.

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin accepted this revision.
mzolotukhin added a comment.
This revision is now accepted and ready to land.

Hi Dehao,

Thanks, the patch mostly looks good to me (see minor remarks inline). I also would like Chandler to take a look at this too, as we discussed this with him in the past - I've added him to reviewers.

Thanks,
Michael



================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:54
+             "unrolling will reduce the total runtime From X to Y, we will "
+             "boost the loop unroll threshold to DefaultThreshold*(X/Y)^2. "
+             "The boost factor should not exceed this parameter in order to "
----------------
The comment is still about quadratic formula:)


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:748
+                ? UP.PercentMaxThresholdBoost
+                : std::min(100 * Cost->RolledDynamicCost / Cost->UnrolledCost,
+                           UP.PercentMaxThresholdBoost);
----------------
This computation potentially may overflow. We probably need some checks against it.


================
Comment at: test/Transforms/LoopUnroll/full-unroll-heuristics-dce.ll:1
-; RUN: opt < %s -S -loop-unroll -unroll-max-iteration-count-to-analyze=100 -unroll-dynamic-cost-savings-discount=1000 -unroll-threshold=10 -unroll-percent-dynamic-cost-saved-threshold=60 | FileCheck %s
+; RUN: opt < %s -S -loop-unroll -unroll-max-iteration-count-to-analyze=100 -unroll-threshold=12 -unroll-max-percent-threshold-boost=400 | FileCheck %s
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
----------------
Minor: if possible, please don't change `-unroll-threshold=12`. We're replacing `-dynamic-cost-savings-discount` and `-unroll-percent-dynamic-cost-saved-threshold`, so the changes should touch only them.


================
Comment at: test/Transforms/LoopUnroll/partial-unroll-const-bounds.ll:24
   %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
-  %exitcond = icmp eq i64 %indvars.iv.next, 10
+  %exitcond = icmp eq i64 %indvars.iv.next, 20
   br i1 %exitcond, label %for.end, label %for.body
----------------
Can we avoid this change?  If no, the comment before the test needs to be updated.


https://reviews.llvm.org/D26989