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

[Amir Ayupov via Phabricator via llvm-commits]

Amir added inline comments.


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:312
+
+  // Start bfs from the source
+  std::queue<uint64_t> Queue;
----------------
spupyrev wrote:
> 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? 
> 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? 

No, I don't have any good examples of using ADT algorithms with built-in types. LLVM classes use them throughout, with examples in `llvm/unittests/ADT/`. I tried using them with our BinaryBasicBlock but we implement GraphTraits interfaces. I guess it's fine to leave the BFS implementation as-is if FlowFunction doesn't already use any of ADT stuff, but in general it's a good idea to reuse LLVM's algorithms if you end up reimplementing them in several places.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144500