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

[Amir Ayupov via Phabricator via llvm-commits]

Amir added a comment.

I'd rather move changes to llvm/lib/Transforms/Utils/SampleProfileInference.cpp to a separate diff to test with existing users and land separately as the changes don't seem to be specific to BOLT.



================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:138
+/// and initialize its jumps and metadata.
+void createFlowFunction(const BinaryFunction::BasicBlockOrderType &BlockOrder,
+                        FlowFunction &Func) {
----------------
Can we return FlowFunction object from this function?


================
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) {
----------------
Do we really need this? since we're calling updateLayoutIndices, we have BB->getLayoutIndex().


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:285-287
+      if (MatchedSrcBlock != nullptr) {
+        OutWeight[MatchedSrcBlock->Index] += YamlSI.Count;
+      }
----------------



================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:289-291
+      if (MatchedDstBlock != nullptr) {
+        InWeight[MatchedDstBlock->Index] += YamlSI.Count;
+      }
----------------



================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:312
+
+  // Start bfs from the source
+  std::queue<uint64_t> Queue;
----------------
Can we use llvm/ADT/GraphTraits + BreadthFirstIterator to avoid reimplementing this?


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:383-386
+  bool HasExitBlocks = false;
+  for (size_t I = 1; I < Func.Blocks.size(); I++) {
+    if (Func.Blocks[I].isExit()) {
+      HasExitBlocks = true;
----------------
Please use `llvm::any_of`


================
Comment at: bolt/lib/Profile/StaleProfileMatching.cpp:505
+        setOrUpdateAnnotation(Instr, "CTCTakenCount", Block.Flow);
+        setOrUpdateAnnotation(Instr, "CTCMispredCount", 0);
+      } else if (BC.MIB->isCall(Instr)) {
----------------
It's just removing CTCMispredCount annotation if it exists.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144500