[PATCH] D144500: [BOLT] initial version of stale profile matching

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added a comment.

Thanks for this patch, Sergey! Now that it's no longer a draft, can you update the title and summary? Do you plan to turn the option on by default in a separate patch?



================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:197
+  for (uint64_t I = 1; I < Func.Blocks.size(); I++) {
+    if (I == 1 || InDegree[I] == 0) {
+      uint64_t Src = 0;
----------------
Some multi-entry functions will have secondary entry points reachable from the "main" entry point. In that case, their `InDegree` will be non-zero. Do you want to capture those here as well? The proper way to check if the block is reachable from outside the function is to check `isEntryPoint()`.


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:348
+  while (!Queue.empty()) {
+    uint64_t Src = Queue.front();
+    Queue.pop();
----------------
nit:


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:351
+    for (FlowJump *Jump : Func.Blocks[Src].PredJumps) {
+      uint64_t Dst = Jump->Source;
+      if (!VisitedExit[Dst]) {
----------------
nit:


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:421
+/// the binary function.
+void assignAnnotations(BinaryFunction &BF,
+                       const BinaryFunction::BasicBlockOrderType &BlockOrder,
----------------
Would `assignProfile()` describe the function more accurately?


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:519
+  // Make sure that block indices and hashes are up to date
+  BF.getLayout().updateLayoutIndices();
+  BF.computeBlockHashes();
----------------
Does the algorithm depend on the layout? I.e. will inferred profile be different if we supply a different order of basic blocks?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144500