[PATCH] D39980: Add heuristics for irreducible loop metadata under PGO

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi added inline comments.


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:1170
+              << getBlockName(HeaderNode) << "\n");
+        HeadersWithoutWeight.insert(H);
         continue;
----------------
davidxl wrote:
> what is the root cause of missing header weight?
General transformations/optimizations that happen after the PGO annotation pass and manipulate the CFG like tail dup, block splitting, etc.


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:1184
     }
+    // As a heuristic, if some headers don't have a weight, give them the
+    // minimium weight seen. If no headers have a weight, give them even weight
----------------
davidxl wrote:
> What is the intuition behind this heuristic?
The idea is to not to disrupt the existing trends too much by using a weight that's in the general range of the other headers' weights (eg. don't want to use a weight of 1000000 when the others have weights in the range of 1-10). The average weight, as opposed to the minimum, would also work in that regard. But based on the performance data, the minimum seems to work better. *Maybe* because the average could be off-base if there are major outliers (a few very hot headers) and the cold headers may be more common than warm/hot headers. Added more to the comment.


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:1196
+            << " to " << getBlockName(HeaderNode) << "\n");
+      if (MinHeaderWeight && MinHeaderWeight.getValue())
+        Dist.addLocal(HeaderNode, MinHeaderWeight.getValue());
----------------
davidxl wrote:
> Is this check needed? MinHeaderWeight is always set.
Done. Still need to check against the zero value which addLocal() has an assert against though.


https://reviews.llvm.org/D39980