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

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev marked 4 inline comments as done.
spupyrev added a comment.

> 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?

For context, this diff alone isn't enough for the feature; we still have https://reviews.llvm.org/D146661, which provides an actual hash computation. I'll adjust the summary to highlight that.
Once both diffs are reviewed, we plan to turn this on for a couple of services and monitor the results. If there are no issues, we can make this on by default.



================
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;
----------------
maksfb wrote:
> 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()`.
These dummy entry blocks are designed exactly to capture extra entry points. From my tests on the clang binary, they do _not_ have jumps from the main entry point, and thus, their InDegree is zero. Perhaps, there are some other cases that I don't see on clang?

Anyhow, for blocks with InDegree>0, we don't need to make special adjustments; they will be handled naturally.


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:519
+  // Make sure that block indices and hashes are up to date
+  BF.getLayout().updateLayoutIndices();
+  BF.computeBlockHashes();
----------------
maksfb wrote:
> Does the algorithm depend on the layout? I.e. will inferred profile be different if we supply a different order of basic blocks?
In theory, the order shouldn't matter much. Here we `updateLayoutIndices` only to be able to index the blocks.

In practice however, there are "almost identical" basic blocks, especially with aggressive inlining. Such blocks have identical bodies, neighbors with identical bodies etc. There is no way to locally determine which blocks belong to which profile item. In order to break ties in such cases, I decided to simply use block addresses (that is, offsets) as a tie breaking rule. It doesn't happen too often but without the tie breaking rule the inference quality is slightly worse. (Using block indices in the layout also works here but provides a tiny regression in comparison to addresses)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144500