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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:49
 
-static cl::opt<unsigned> UnrollPercentDynamicCostSavedThreshold(
-    "unroll-percent-dynamic-cost-saved-threshold", cl::init(50), cl::Hidden,
-    cl::desc("The percentage of estimated dynamic cost which must be saved by "
-             "unrolling to allow unrolling up to the max threshold."));
-
-static cl::opt<unsigned> UnrollDynamicCostSavingsDiscount(
-    "unroll-dynamic-cost-savings-discount", cl::init(100), cl::Hidden,
-    cl::desc("This is the amount discounted from the total unroll cost when "
-             "the unrolled form has a high dynamic cost savings (triggered by "
-             "the '-unroll-perecent-dynamic-cost-saved-threshold' flag)."));
+static cl::opt<unsigned> PercentMaxThresholdBoost(
+    "unroll-max-percent-threshold-boost", cl::init(400), cl::Hidden,
----------------
Here and above, if the value is a limit or max, start with that. So I would say `MaxPercentThresholdBoost`. In fact, the flag string below is already that order.

Also, I liked having these be different from the names in the TTI struct. Can you keep the `Unroll` prefix that used to be here? That also matches the flag text.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:51-56
+    cl::desc("Boost the threshold for complete unroll by a max percent to "
+             "allow aggressive unrolling to reduce run time. If completely "
+             "unrolling will reduce the total runtime From X to Y, we will "
+             "boost the loop unroll threshold to DefaultThreshold*(X/Y). "
+             "The boost factor should not exceed this parameter in order to "
+             "prevent from excessive code bloat."));
----------------
The text here doesn't really parse for me. It starts off talking about something other than what it is. How about:

  The maximum 'boost' (represented as a percentage >= 100) applied to
  the threshold when aggressively unrolling a loop due to the dynamic
  cost savings. If completely unrolling a loop will reduce the total
  runtime from X to Y, we boost the lop unroll threshold to
  DefaultThreshold*std::min(MaxPercentThresholdBoost, X/Y). This limit
  avoids excessive code bloat.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:745-751
+        unsigned BoostingFactorPercent = UP.PercentMaxThresholdBoost;
+        if (Cost->RolledDynamicCost >= UINT_MAX / 100)
+          BoostingFactorPercent = 100;
+        else if (Cost->UnrolledCost != 0)
+          BoostingFactorPercent =
+              std::min(100 * Cost->RolledDynamicCost / Cost->UnrolledCost,
+                       UP.PercentMaxThresholdBoost);
----------------
How about factoring the logic to compute this BoostFactorPercent into a helper function? I think that would make it more clear -- you could use early return to bail out in error conditions.

I think it would also be a good place to explain some of the reasoning behind the specific formula (runtime cost / unrolled cost) used to scale the threshold.


https://reviews.llvm.org/D26989