[PATCH] D90125: [CSSPGO] Infrastructure for context-sensitive Sample PGO and Inlining

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/SampleContextTracker.h:70
+  // Map line+discriminator location to child context
+  std::map<uint32_t, ContextTrieNode> ChildContext;
+
----------------
Rename it to ChildContexts or AllChildContext?


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:61-65
+ContextTrieNode *
+ContextTrieNode::getOrCreateChildContext(const LineLocation &CallSite,
+                                         StringRef CalleeName) {
+  return getOrCreateChildContext(CallSite, CalleeName, true);
+}
----------------
Make param AllowCreate default to true so we don't need this wrapper?


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:71
+  uint32_t Hash = nodeHash(NodeToMove.getFuncName(), CallSite);
+  assert(!ChildContext.count(Hash));
+  LineLocation OldCallSite = NodeToMove.CallSiteLoc;
----------------
Add an assert message. 


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:269
+      ContextTrieNode &ToNode = promoteMergeContextSamplesTree(*FromNode);
+      assert(!Node || Node == &ToNode);
+      Node = &ToNode;
----------------
Add an assertion message.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:283
+    const FunctionSamples *InlinedSamples) {
+  assert(InlinedSamples);
+  LLVM_DEBUG(dbgs() << "Marking context profile as inlined: "
----------------
Add an assertion message.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:357
+                                          StringRef CalleeName) {
+  assert(DIL);
+
----------------
Add an assertion message.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:373
+
+ContextTrieNode *SampleContextTracker::getContextFor(const DILocation *DIL) {
+  assert(DIL);
----------------
A question, the context in SampleContextTracker includes not only inline stack but also call stack. S vector below only contains the inline stack at the DIL location. How can it match with the full stack starting from RootContext?  


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:437
+
+  assert(!AllowCreate || ContextNode);
+  return ContextNode;
----------------
Add an assertion message.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:446
+ContextTrieNode &SampleContextTracker::addTopLevelContextNode(StringRef FName) {
+  assert(!getTopLevelContextNode(FName));
+  return *RootContext.getOrCreateChildContext(LineLocation(0, 0), FName);
----------------
Add an assertion message.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:453
+    StringRef ContextStrToRemove) {
+  assert(!ContextStrToRemove.empty());
+
----------------
Add assertion message.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:462
+  if (!MoveToRoot) {
+    NewCallSiteLoc = FromNode.getCallSiteLoc();
+  }
----------------
Use OldCallSiteLoc instead?


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:473-490
+    // Destination node exists, merge samples for the context tree
+    FunctionSamples *FromSamples = FromNode.getFunctionSamples();
+    FunctionSamples *ToSamples = ToNode->getFunctionSamples();
+    if (FromSamples && ToSamples) {
+      // Merge/duplicate FromSamples into ToSamples
+      ToSamples->merge(*FromSamples);
+      ToSamples->getContext().setState(SyntheticContext);
----------------
It will be slightly easier to read if the block can be extracted to a function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90125