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

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin added a comment.

Hi Dehao,

Please find some comments inline.

Thanks,
Michael



================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:245-249
+    /// If complete unrolling will reduce the cost of the loop, we will boost
+    /// the Threshold by a certain percent to allow more aggressive complete
+    /// unrolling. This value provides the maximum boost percentage that we
+    /// can apply to Threshold (The value should be no less than 100).
+    unsigned PercentMaxThresholdBoost;
----------------
mzolotukhin wrote:
> What is the formula for the threshold boost? Could you include it into the comment?
I would also add a couple of examples to clarify the intention of this boost. Like if unrolling reduces the loop (in terms of execution time) by a factor of 4x, then we boost the threshold by the factor of 4. If unrolling isn't expected to reduce the running time, then we don't increase the threshold.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:49-52
+static cl::opt<unsigned> PercentMaxThresholdBoost(
+    "unroll-max-percent-threshold-boost", cl::init(400), cl::Hidden,
+    cl::desc("Boost the threshold for complete unroll by a max percent to "
+             "allow aggressive unrolling to reduce run time"));
----------------
I think this description might be unclear for future users who are not familiar with this patch. We need to reflect what, when, and how this parameter boosts.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:745-746
+                : Cost->RolledDynamicCost * 100 / Cost->UnrolledCost;
+        unsigned ThresholdBoost = std::min(Benefit * Benefit / 10000,
+                                           UP.PercentMaxThresholdBoost / 100);
+        if (Cost->UnrolledCost < UP.Threshold * ThresholdBoost) {
----------------
1) Why do we use `Benefit*Benefit` here?
2) The result is `unsigned`, which means that the values we can get here are very limited (1,2,3, or 4 with `PercentMaxThresholdBoost` = 400). I'd suggest computing the upper bound for the cost, not the benefit to work around it.

Also, this code needs some comments, it's not obvious what we're doing here.


================
Comment at: test/Transforms/LoopUnroll/full-unroll-crashers.ll:2
 ; Check that we don't crash on corner cases.
-; RUN: opt < %s -S -loop-unroll -unroll-max-iteration-count-to-analyze=1000 -unroll-threshold=1 -unroll-percent-dynamic-cost-saved-threshold=20 -o /dev/null
+; RUN: opt < %s -S -loop-unroll -unroll-max-iteration-count-to-analyze=1000 -unroll-threshold=1 -o /dev/null
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
----------------
Don't we need a corresponding `-unroll-max-percent-threshold-boost` argument here? We need to explicitly pass it so that the test is immune to future changes in default thresholds.


================
Comment at: test/Transforms/LoopUnroll/full-unroll-heuristics-2.ll:1
-; RUN: opt < %s -S -loop-unroll -unroll-max-iteration-count-to-analyze=1000 -unroll-threshold=10  -unroll-percent-dynamic-cost-saved-threshold=70 -unroll-dynamic-cost-savings-discount=90 | FileCheck %s
+; RUN: opt < %s -S -loop-unroll -unroll-max-iteration-count-to-analyze=1000 -unroll-threshold=10 | FileCheck %s
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
----------------
Same here and in other tests.


https://reviews.llvm.org/D26989