[PATCH] D147812: [InstrProf] Use BalancedPartitioning to order temporal profiling trace data

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev added inline comments.


================
Comment at: llvm/include/llvm/Support/BalancedPartitioning.h:88
+  /// The depth of the recursive bisection
+  uint32_t SplitDepth = 16;
+  /// The maximum number of bp iterations per split
----------------
I thought we use `18` by default?


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:85
+
+void BalancedPartitioning::BPThreadPool::wait() {
+  // TODO: We could remove the mutex and condition variable and use
----------------
Would be great to unify the implementation with the existing ThreadPool, but I don't have a suggestion on how exactly to implement that. I'm fine with keeping it as is, if there are no alternative suggestions.


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:214
+  // is 4x larger than the number of FunctionNodes.
+  SignatureMapT Signatures(/*InitialReserve=*/NumNodes * 4);
+  for (auto &N : Nodes) {
----------------
Do we want to get rid of the map? (and the weird 4x initialization)


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:366
+double
+BalancedPartitioning::computeGoal(const SignatureMapT &Signatures) const {
+  double Sum = 0;
----------------
I think we've got rid of this method


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:3046
+      "cutoffs", cl::CommaSeparated,
+      cl::desc("Timestamp cutoff values used to initialize function nodes for "
+               "ordering functions"),
----------------
davidxl wrote:
> ellis wrote:
> > davidxl wrote:
> > > How are the default values selected?
> > These values were also empirically found by optimizing a large binary. I was debating leaving out a default value since these values are pretty specific to the binary we tested. Now that I think about it, we can try to derive a similar list by looking at the total number of functions and assign cutoffs linearly.
> or add some empirical guidance in the description.
The assumption here is that most of the traces are of size ~32K. Perhaps we could also add a comment and a clarification to re-consider the values, if the traces are of significantly different sizes?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147812/new/

https://reviews.llvm.org/D147812