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

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev marked 6 inline comments as done.
spupyrev added inline comments.


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:146
+  // Create FlowBlock for every basic block in the binary function
+  DenseMap<const BinaryBasicBlock *, uint64_t> BlockIndex;
+  for (const BinaryBasicBlock *BB : BlockOrder) {
----------------
Amir wrote:
> Do we really need this? since we're calling updateLayoutIndices, we have BB->getLayoutIndex().
great, thanks


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:312
+
+  // Start bfs from the source
+  std::queue<uint64_t> Queue;
----------------
Amir wrote:
> Can we use llvm/ADT/GraphTraits + BreadthFirstIterator to avoid reimplementing this?
If I understand correctly, that would require adding some boilerplate code for iterators over the nodes and their neighbors? In addition, I need to run an "inverse" BFS starting from all reachable sinks... It certainly can be done, but I'm not sure if the implementation would be any simpler. Is there a good example of how to implement something similar with built-in types? 


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:505
+        setOrUpdateAnnotation(Instr, "CTCTakenCount", Block.Flow);
+        setOrUpdateAnnotation(Instr, "CTCMispredCount", 0);
+      } else if (BC.MIB->isCall(Instr)) {
----------------
Amir wrote:
> It's just removing CTCMispredCount annotation if it exists.
which is exactly what we're trying to accomplish, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144500