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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:361
+// Example of full context string (note the wrapping `[]`):
+//    `[main:3 @ _Z5funcAi:1 @ _Z8funcLeafi]`
+// Example of context-less function name (same as AutoFDO):
----------------
davidxl wrote:
> wenlei wrote:
> > wenlei wrote:
> > > davidxl wrote:
> > > > The syntax of the context string can probably be made more consistent like:
> > > > 
> > > > SampleContext : LeafContext
> > > >                            : ParentContext   LeafContext
> > > > 
> > > > LeafContext: function_name
> > > > ParentContext: [ParentFrames]
> > > > ParentFrames:   OneParentFrame   
> > > >                         : ParentFrames   OneParentFrame
> > > > OneParentFrame:  function_name:line @
> > > > 
> > > > So in your example, the full context string should look like:
> > > > 
> > > > [main:3 @_Z5funcAi:1 @] _Z8funcLeafi
> > > > 
> > > Agreed, that's indeed more consistent and I like that better. Thanks for the suggestion. Will make the change (llvm-profgen change will follow too).
> > Actually, there're other implication if we were to change context string to be `[context] leaf`. With the currently syntax, when we promote context, new context string is a substring of the original one. So we just create StringRef wrapper for context promotion without creating new strings. 
> > 
> > E.g. when `main:3 @ _Z5funcAi:1 @ _Z8funcLeafi` is promoted, it becomes `_Z5funcAi:1 @ _Z8funcLeafi` which is sub string of the original one (StringRef that reuses the underlying string).
> > 
> > If we use `[main:3 @_Z5funcAi:1 @] _Z8funcLeafi`, promotion will lead to something like `[_Z5funcAi:1 @] _Z8funcLeafi`, which is no longer a substring and a new string need to be created. We use two StringRef to represent context part and leaf part today, but we also need a consistent string representation for the full context `getNameWithContext`.
> > 
> > So practically, current syntax is more efficient for context promotion.
> > 
> > Additionally, with the proposed syntax, top level context would look like this `[] main` to differentiate from context-less header. In this case, `[main]` is probably better.
> > 
> > What do you think?
> > 
> > Actually, there're other implication if we were to change context string to be `[context] leaf`. With the currently syntax, when we promote context, new context string is a substring of the original one. So we just create StringRef wrapper for context promotion without creating new strings. 
> > 
> > E.g. when `main:3 @ _Z5funcAi:1 @ _Z8funcLeafi` is promoted, it becomes `_Z5funcAi:1 @ _Z8funcLeafi` which is sub string of the original one (StringRef that reuses the underlying string).
> 
> In this case, where does the bracket go?
> 
> > 
> > If we use `[main:3 @_Z5funcAi:1 @] _Z8funcLeafi`, promotion will lead to something like `[_Z5funcAi:1 @] _Z8funcLeafi`, which is no longer a substring and a new string need to be created. We use two StringRef to represent context part and leaf part today, but we also need a consistent string representation for the full context `getNameWithContext`.
> > 
> > So practically, current syntax is more efficient for context promotion.
> > 
> > Additionally, with the proposed syntax, top level context would look like this `[] main` to differentiate from context-less header. In this case, `[main]` is probably better.
> 
> what is the difference between top level context vs context less header?
> 
> 
> > 
> > What do you think?
> > 
> 
> 
> In this case, where does the bracket go?

Internally, we don't use the bracket, and since they're the first and last character, we just create a StringRef with bracket removed for `SampleContext`, without creating new string.

So in profile file, we have `[main:3 @ _Z5funcAi:1 @ _Z8funcLeafi]`, while in `SampleContext` used by LLVM and tools, we have `main:3 @ _Z5funcAi:1 @ _Z8funcLeafi`. The form used by `SampleContext` is easier for context promotion, and the conversion from `[]` form to `SampleContext` is just a StringRef wrapper.

> what is the difference between top level context vs context less header? 

They have different meanings, consider a dso, context-less profile `foo` means we just don't know the calling context, while context profile `[foo]` means this is called directly from external function. 

In practice, we don't have context-less profile and context profile in a single profile file now, so it's also about consistency in context profile and enough differentiation between context profile and context-less profile (i.e reserve the form `main` *only* for context-less profile for today's AFDO).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90125