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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:311
+         !FProfile->getContext().hasState(InlinedContext))) {
+      Node->setFunctionSamples(nullptr);
     }
----------------
wlei wrote:
> 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)
> 
> 
>  it depends on the FullContext, now we don't have it, so I changed to Node->getParentContext() != &ContextTracker.getRootContext()

I see. That makes sense.

> It's a bit tricky, the FunctionSample can be from two sources

It is tricky. 

Do we have any memory leakage by resetting the pointer to null but not freeing the data? If so, we may fail the sanitizer tests.
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031