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

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:252-253
+    ///                                    PercentMaxThresholdBoost)
+    /// E.g. if complete unrolling reduces the loop execution time by 50%
+    /// then we boost the threshold by the factor of 4x. If unrolling is not
+    /// expected to reduce the running time, then we do not increase the
----------------
mzolotukhin wrote:
> Shouldn't it be 2x instead of 4x?
Updated the comment to make it consistent.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:54
+             "unrolling will reduce the total runtime by X%, we will boost "
+             "the loop unroll threshold to (1/(1-X%))^2. The boost factor "
+             "should not exceed this parameter in order to prevent from "
----------------
mzolotukhin wrote:
> I didn't realize that we use this formula - I thought we're using a linear function. Doesn't it contradict to this?
> ```
>     /// BoostedThreshold = Threshold * min(RolledCost / UnrolledCost,
>     ///                                    PercentMaxThresholdBoost)
> ```
Updated the comment to make it consistent.


================
Comment at: test/Transforms/LoopUnroll/partial-unroll-const-bounds.ll:1
-; RUN: opt < %s -S -unroll-threshold=20 -loop-unroll -unroll-allow-partial -unroll-runtime -unroll-allow-remainder -unroll-dynamic-cost-savings-discount=0 | FileCheck %s
+; RUN: opt < %s -S -unroll-threshold=20 -loop-unroll -unroll-allow-partial -unroll-runtime -unroll-allow-remainder -unroll-max-percent-threshold-boost=200 | FileCheck %s
 
----------------
mzolotukhin wrote:
> I think `unroll-max-percent-threshold-boost` should be `100` to correspond to `unroll-dynamic-cost-savings-discount=0`. BTW, it looks a bit weird that value `100` for the boost means that we are actually not boosting anything.
Yes, 100 means 100%, i.e no boosting. Maybe we need to change the naming to make it more accurate, suggestions?


https://reviews.llvm.org/D26989