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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:311
+         !FProfile->getContext().hasState(InlinedContext))) {
+      Node->setFunctionSamples(nullptr);
     }
----------------
hoy wrote:
> 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.
>  
Okay.. The `FuncToCtxtProfiles` also hold the pointers but eventually after ContextTracker is freed, it's still memory leakage.. let me try to free them. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031