[PATCH] D103289: A post-processing for BFI inference

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev marked an inline comment as done.
spupyrev added inline comments.


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1440
+    uint64_t IntValue = uint64_t(Value * MAX_INT);
+    return Scaled64::getFraction(IntValue, MAX_INT);
+  };
----------------
davidxl wrote:
> why not using ScaledNumber::get(uint64_t) interface?
here we convert double `EPS = 1e-12` to Scaled64, so need some magic. `ScaledNumber::get(uint64_t)` won't work for values < 1


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1443
+  const auto Threshold = doubleToScaled(IterativeBFIThreshold);
+  const size_t MaxIterations = IterativeBFIMaxIterations * Freq.size();
+
----------------
davidxl wrote:
> why multiplying by Freq.size()? Should the option description reflect this?
good point! I renamed the option and adjusted the description


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1451
+  // Successors[I] holds unique sucessors of the I-th block
+  auto Successors = std::vector<std::vector<size_t>>(Freq.size());
+  for (size_t I = 0; I < Freq.size(); I++) {
----------------
davidxl wrote:
> Can this loop be moved into computation of probMatrix and pass the succ vector in to avoid redundant computation.
Here `Successors` represent successors of each vertex in the (auxiliary) graph. It is different from `Succs` object in the original CFG. (In particular the auxiliary graph contains jumps from all exit block to the entry)

Also I find the current interface a bit cleaner: the main inference method, `iterativeInference`, takes the probability matrix as input and returns computed frequencies. `Successors` is an internal variable needed for computation.


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1489
+    }
+    if (OneMinusSelfProb != Scaled64::getOne())
+      NewFreq /= OneMinusSelfProb;
----------------
davidxl wrote:
> Does it apply to other backedges too?
not sure I fully understand the question, but we need an adjustment only for self-edges; blocks without self-edges don't need any post-processing

I added a short comment before the loop


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103289