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

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev added inline comments.


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1388
+  // Extract initial frequencies for the blocks
+  DenseMap<const BlockT *, size_t> BlockIndex;
+  auto Freq = std::vector<Scaled64>(HotBlocks.size());
----------------
davidxl wrote:
> why is this map needed (which adds a layer of indirection)?
The map is used to index successors/predecessors of "hot" blocks, see line 1603.

As an optimization, we don't process all the blocks in a function but only those that can be reached from the entry via branches with a positive probability. These are `HotBlocks` in the code. Typically, the number of HotBlocks is 2x-5x smaller than the total number of blocks in the function. In order to find an index of a block within the list, we either need to do a linear scan over HotBlocks, or have such an extra map.


================
Comment at: llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h:1397
+  }
+  if (SumFreq.isZero())
+    return;
----------------
davidxl wrote:
> is it possible, given the blocks are hot?
In theory, there is no guarantee that at least one of `getFloatingBlockFreq` is non-zero. (Notice that our "definition" of hot blocks does not rely on the result of the method).

In practice, I've never seen this condition satisfied in our extensive evaluation. So let me change it to an assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103289