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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1439
+    const uint64_t MAX_INT = ((uint64_t)1) << 40;
+    uint64_t IntValue = uint64_t(Value * MAX_INT);
+    return Scaled64::getFraction(IntValue, MAX_INT);
----------------
can this overflow?


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1440
+    uint64_t IntValue = uint64_t(Value * MAX_INT);
+    return Scaled64::getFraction(IntValue, MAX_INT);
+  };
----------------
why not using ScaledNumber::get(uint64_t) interface?


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1443
+  const auto Threshold = doubleToScaled(IterativeBFIThreshold);
+  const size_t MaxIterations = IterativeBFIMaxIterations * Freq.size();
+
----------------
why multiplying by Freq.size()? Should the option description reflect this?


================
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++) {
----------------
Can this loop be moved into computation of probMatrix and pass the succ vector in to avoid redundant computation.


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1489
+    }
+    if (OneMinusSelfProb != Scaled64::getOne())
+      NewFreq /= OneMinusSelfProb;
----------------
Does it apply to other backedges too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103289