[PATCH] D144500: [BOLT] stale profile matching [part 1 out of 2]

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added inline comments.


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:262-263
+      // assign the jump weight based on the profile count
+      uint64_t SrcIndex = YamlBB.Index;
+      uint64_t DstIndex = YamlSI.Index;
+
----------------
nit:


================
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;
----------------
spupyrev wrote:
> 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.
Secondary entry points normally come from assembly code, perhaps from some Fortran programs as well. HHVM has memcpy written like that: https://github.com/facebook/hhvm/blob/master/hphp/util/memcpy-x64.S


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:519
+  // Make sure that block indices and hashes are up to date
+  BF.getLayout().updateLayoutIndices();
+  BF.computeBlockHashes();
----------------
spupyrev wrote:
> 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)
I see. So then `BlockOrder` is used only for mapping blocks in `BinaryFunction` to `FlowFunction` and back?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144500