[PATCH] D127031: [CSSPGO][llvm-profgen] Reimplement SampleContextTracker using context trie

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:701
+  // by the path from root to this node.
+  ContextTrieNode *ContextNode;
   // State of the associated sample profile
----------------
hoy wrote:
> The field is only useful during context manipulation. Is it possible to store the link in an auxiliary map whereever it is used?
I think we use context for two things 1)context manipulation(inlining) 2) reader and writer based on ProfileMap.
My thought is `ContextNode` is the same thing to `FullContext`, i,e. we can always start from this node to iterate trie reversely to get the array of frames. I was wondering if we can use `ContextNode` to replace the `FullContext` in the future. And for sample reader and writer things, the key of the map can work as the FullContext, we might not need this field either. That's why I'm thinking to keep it here. WDYT?


================
Comment at: llvm/include/llvm/Transforms/IPO/SampleContextTracker.h:106
   // deterministically.
-  using ContextSamplesTy = std::set<FunctionSamples *, ProfileComparer>;
 
----------------
wenlei wrote:
> Is std::set unnecessary or is it wrong? 
Before it sorted the elements by ` A->getContext() < B->getContext();`, but now `getContext` is an empty context, so it ends up with non-deterministic issue and losing elements. While we can recover the context by waling through the trie, but thinking using `SmallVector` is enough here, the order of traversing trie is deterministic(`getAllChildContext()` is deterministic).


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:93
       FSamples->getContext().setState(SyntheticContext);
-      LLVM_DEBUG(dbgs() << "  Context promoted to: "
-                        << FSamples->getContext().toString() << "\n");
----------------
hoy wrote:
> may still be helpful to keep the debug dump?
`moveToChildContext` is the function of `ContextTrieNode`, the `getContextString`  is in `SampleContextTracker`. Then it requires to pass `SampleContextTracker` to the `moveToChildContext` function. 

I just realize before it dump the promotion result for each node, but now we don't have the concept of promotion. Maybe dumping the the head node(ToNode) is enough?




================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:209
     NewNode->setFunctionSamples(FSamples);
+    FSamples->setContextNode(NewNode);
+  }
----------------
hoy wrote:
> Is this also done in `populateFuncToCtxtMap`?
Good catch!


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:389
+      // Skip inlined context profile
+      if (ContextOnNode.hasState(InlinedContext))
+        continue;
----------------
wenlei wrote:
> Why we need to check MergedContext on OldContext, but InlinedContext on ContextOnNode?
Let me demo by an example.

Given two node with same name `foo`, say N1 and N2. N1 own a FunctionSample S1, and N2 own S2, i, e. S1->N1, S2->N2 

Supposing during merge `bar`, `foo` is also merge, say N2 --> N1, then S2's context is marked `MergedContext` and S2's is update to point to N1. S1 still point to N1.

Then when call getBaseSample for `foo` to merge all nodes, S2 and S1 both point to N1 now, we can just skip S2 which contains the OldContext marked `MergedContext `.
















================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:470
+SampleContextTracker::getContextString(ContextTrieNode *Node) const {
+  SmallVector<SampleContextFrame, 10> Res;
+  if (Node == &RootContext)
----------------
hoy wrote:
> nit: use SampleContextFrameVector
Done, thanks!


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:472
+  if (Node == &RootContext)
+    return "";
+  Res.emplace_back(Node->getFuncName(), LineLocation(0, 0));
----------------
wenlei wrote:
> `return string();`
Done, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031