[PATCH] D19049: [BlockFrequencyInfo] Handling nested irreducible CFG with geometric series and top-down prorogation

[Than McIntosh via llvm-commits]

thanm added a comment.

Hello,

Nice work, it was interesting to poke through this code and learn a bit about how block frequencies are calculated.

General remarks:

- as w/ previous commenters I think it would be nice to have some concrete case from a real benchmark (spec or otherwise) where you can point to an improvement of some sort as a result of this change

- the original version of the code makes reference to the idea of computing block frequencies using infinite geometric series (original BlockFrequencyInfoImpl.h lines 757, 786), but it is only in the test case (test/Analysis/BlockFrequencyInfo/irreducible.ll) where an actual example is provided along with its associated geometric series.  Given that the new implementation is actually doing computing series start terms and ratios, I think it would make much more sense (from a readability perspective) to promote the comments/examples from the test case to the actual code itself.

I've posted more specific nits/comments inline on the Phab review page.

Thanks, Than


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:723-726
@@ -756,6 +722,6 @@
 ///
 ///     Modelling irreducible control flow exactly involves setting up and
 ///     solving a group of infinite geometric series.  Such precision is
 ///     unlikely to be worthwhile, since most of our algorithms give up on
 ///     irreducible control flow anyway.
 ///
----------------
This comment seems to now be stale -- rewrite?

================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:843
@@ +842,3 @@
+  /// Propagate mass on each node of \c Loop in topological order. For
+  /// subloops, recurisvely call \c computeIrreducibleMass to calculate its
+  /// local frequency and exits weights distribution. Store calculated terms
----------------
recurisvely => recursively

================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:1192
@@ +1191,3 @@
+    auto It = std::find(SortedNodes.begin(), SortedNodes.end(), HeaderNode);
+    for (int i = 0; i < SortedNodes.size(); ++i, ++It) {
+      if (It == SortedNodes.end())
----------------
size_t would be a better type for "i" (when I do a build with this patch I get a warning about signed/unsigned comparison)


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:1274
@@ +1273,3 @@
+
+  // To calculate the geometirc series, we need start term(a) and ratio(r). The
+  // final block freq is a/(1-r).
----------------
typo geometirc => geometric


http://reviews.llvm.org/D19049