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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Thanks for the very helpful description!



================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:504-506
+      // For CSFDO, in order to conserve profile size, we no longer write out
+      // locations profile for those not hit during training, so we need to
+      // treat them as zero instead of error here.
----------------
davidxl wrote:
> CSFDO ==> CSSPGO
It means CSSPGO will treat all the new lines as cold, even if some of them may be inferred from other parts of the profile. How much extra size is needed if zero is emitted? 


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:246
+  LLVM_DEBUG(dbgs() << "Getting base profile for function: " << Name << "\n");
+  ContextTrieNode *Node = getTopLevelContextNode(Name);
+  if (MergeContext) {
----------------
I don't understand what the top level means here. Better document it. 

Do we cache the base profile somewhere or we merge it everytime?


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:368
+
+ContextTrieNode *SampleContextTracker::getContextFor(const DILocation *DIL) {
+  assert(DIL);
----------------
Do we need to call getCanonicalFnName here to make the name in inline stack canonical so we can match the name in inline stack with the name in context?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1910-1914
+    // In order to be able to fully leverage context-sensitive profile,
+    // we need global top-down inlining, thus do not load profile or
+    // perform profile guided inlining pre-LTO.
+    if (IsThinLTOPreLink)
+      return false;
----------------
Here it means no any profile loading or just no CS profile? ThinLTO thinlink phase needs to know which functions are hot and it can import them, so profile information is needed in ThinLTO prelink.


================
Comment at: llvm/test/Transforms/SampleProfile/Inputs/profile-context-tracker.prof:27
+ 3: 12
+[externalA:17 @ _Z5funcBi]:120:3
+ 0: 3
----------------
Here "main" doesn't show up in the context. Is it a problem of unwinding or debug info?


================
Comment at: llvm/test/Transforms/SampleProfile/profile-context-tracker-debug.ll:1-3
+; REQUIRES: asserts
+; Test for CSSPGO's SampleContextTracker to make sure context profile tree is promoted and merged properly
+; based on inline decision, so post inline counts are accurate.
----------------
Is there anything which cannot be tested in profile-context-tracker.ll? The debug message is usually used as last resort if something cannot be fully tested by just checking IR.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90125