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

[Amir Ayupov via Phabricator via llvm-commits]

Amir added a comment.

I'm trying to see if we can reuse SampleProfileInference code as much as possible given it's already templated and used with LLVM IR/MIR (Flow-Sensitive Sample AutoFDO and Context-Sensitive Sample PGO respectively).



================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:139
+FlowFunction
+createFlowFunction(const BinaryFunction::BasicBlockOrderType &BlockOrder) {
+  FlowFunction Func;
----------------
What's the primary motivation behind adding a BinaryFunction-specific version instead of using `SampleProfileInference<BT>::initFunction`?
Is it the fact that BF can have multiple entry points hence we need a dummy source node, and/or custom handling of EH control flow?


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:161
+  std::vector<uint64_t> InDegree(Func.Blocks.size(), 0);
+  std::vector<uint64_t> OutDegree(Func.Blocks.size(), 0);
+  for (const BinaryBasicBlock *SrcBB : BlockOrder) {
----------------
What outdegree is used for?


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:312
+
+  // Start bfs from the source
+  std::queue<uint64_t> Queue;
----------------
Amir wrote:
> 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.
Does it make a difference to use BFS instead of DFS if the goal is collecting (un)reachable blocks?

There's an overlap in functionality with `SampleProfileInference<BT>::apply` which finds nodes reachable from source and sink using `depth_first_ext` and `inverse_depth_first_ext`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144500