[PATCH] D54223: WIP... [SimpleLoopUnswitch] adding cost multiplier to cap exponential unswitch with

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Honestly, this seems like a really reasonable approach in practice.

Between the sibling threshold and the candidate threshold I think gives highly stable results as well as pruning unreasonable search spaces.

I think this does in practice prevent the explosion, not just make it less likely.

One thing that might be interesting to consider is whether unswitching that typically creates siblings could through some surprise create non-sibling loops due to restructuring of the nest? If not, then this seems pretty solid. If so, we could consider look at the sibling loop nest sizes rather than just the sibling loop nest counts.

Can a contrived test case hit this quickly enough to make sense to add to the tests and show the limit being applied? Potentially by lowering these thresholds on the commandline?



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2486-2487
+    // idea to account for previous unswitches that already happened on this
+    // cluster of loops. Formula is kept intentionally simple since it a cap for
+    // worst cases and not an attempt to make exact size predictions.
+    int CostMultiplier = 1;
----------------
s/since it a/since it is a/

or maybe....

"since it is designed to limit the worst case behavior and not an attempt to provide a nuanced heuristic size prediction"


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2493-2494
+                                   : std::distance(LI.begin(), LI.end()));
+      // Applying a "bottom cap" to allow a certain amount of unswitch
+      // candidates before going into prohibitive power-of-two.
+      unsigned CandidatesPower = std::max(
----------------
Maybe:

"When the number of unswitch candidates is below our "bottom cap" we disable the scaling of the cost and use the directly computed cost."


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2496
+      unsigned CandidatesPower = std::max(
+          int(UnswitchCandidates.size() - (int)UnswitchCandidatesBottomCap), 0);
+
----------------
I would just cast the size to an int rather than the larger function style cast?


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2503-2504
+                   1);
+      // Handle possible overflow. UnswitchThreshold is a good upper boundary
+      // since multiplying by it will definitely move cost over the threshold.
+      if (CandidatesPower > Log2_32(UnswitchThreshold) ||
----------------
Maybe:

"Handle possible overlow."
->
"Compute the cost multiplier in a way that won't overflow by saturating at an upper bound."


Repository:
  rL LLVM

https://reviews.llvm.org/D54223