[PATCH] D95547: [CSSPGO] Support of CS profiles in extended binary format.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:442
   bool isBaseContext() const { return CallingContext.empty(); }
   StringRef getName() const { return Name; }
   StringRef getCallingContext() const { return CallingContext; }
----------------
wmi wrote:
> Nit: I feel getNameWithoutContext is more straightforward.
you mean for the `getName` function?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:454
     assert(!ContextStr.empty());
+    FullContextWithBracket = ContextStr;
     // Note that `[]` wrapped input indicates a full context string, otherwise
----------------
wenlei wrote:
> hoy wrote:
> > wlei wrote:
> > > Should we merge it to the condition in line 462. because as its name "WithBracket", it should always have bracket or we can add a check for the `HasContext`.
> > > ```
> > > if (HasContext) {
> > >   FullContextWithBracket = ContextStr;
> > > }
> > > ```
> > Good point. It really refers to the original input context so that we can use it unconditionally for non-CS profile as well. @wenlei thinking about renaming it OrigContext, what do you think?
> Good catch, in this case OrigContext or InputContext makes more sense.. Sorry for the confusion. 
Sound good. Renamed to `InputContext`.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:685-686
       GUIDToFuncNameMap = Other.GUIDToFuncNameMap;
-
+    if (Context.getNameWithContext(true).empty())
+      Context = Other.getContext();
     if (FunctionHash == 0) {
----------------
wmi wrote:
> Is it possible for the two FunctionSamples candidates to have different non empty contexts?
This `merge` function is called under the assumption that `Other` has the same context with `this` or `this` is empty, which basically means we are initializing `this` with `Other`.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:547
+  SampleContext FContext(*FName);
+  Profiles[FContext] = FunctionSamples();
+  FunctionSamples &FProfile = Profiles[FContext];
----------------
wmi wrote:
> This is unnecessary. The next statement will create the default entry if the key doesn't exist in the map.
Good catch. Will remove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95547