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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:380
 
+      SampleContext &ContextOnNode =
+          FromNode->getFunctionSamples()->getContext();
----------------
hoy wrote:
> ContextOnNode and OldContext should be the same thing now?
Yeah, new the context node is always point to the FunctionSamples, and vice versa. Recovered the original code.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:602
       ToSamples->getContext().setAttribute(ContextShouldBeInlined);
+    FromNode.setFunctionSamples(nullptr);
   } else if (FromSamples) {
----------------
hoy wrote:
> This is no longer needed?
This was to be consistent with the branch below(line:608), I just realized that actually the `FromNode` will be  removed from the trie, they both can be removed, removing them.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:311
+         !FProfile->getContext().hasState(InlinedContext))) {
+      Node->setFunctionSamples(nullptr);
     }
----------------
hoy wrote:
> Should we just keep the original condition checks? 
> 
> Also, where do we actually deallocate the function profile?
> Should we just keep the original condition checks?
do you mean the `isBaseContext`?, it depends on the `FullContext`, now we don't have it, so I changed to `Node->getParentContext() != &ContextTracker.getRootContext()`

>Also, where do we actually deallocate the function profile?
It's a bit tricky, the FunctionSample can be from two sources
1) the llvm-sample-profile, the FunctionSamples's lifetime will be along with the reader.  we can't explicitly call `delete` on this.

2) ProfileGenerator( by the `new`), we need to explicitly call `delete`, I haven't came up with a way to deallocate this yet.  The Node is removed from trie dynamically, I was thinking to use `FuncToCtxtProfiles` to get all FunctionSamples's pointer but this doesn't include the base context.

 (I leave a TODO for this)




================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:164
   // closest matching context.
-  uint32_t getFuncSizeForContext(const SampleContext &Context);
+  uint32_t getFuncSizeForContext(ContextTrieNode *Context);
 
----------------
hoy wrote:
> nit: use const for the parameter
Done!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031