[PATCH] D89723: [CSSPGO][llvm-profgen]Context-sensitive profile data generation

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/noinline-cs-noprobe.test:7
+; CHECK: 3: 1756 bar:1745
+; CHECK:[main:1 @ foo:3 @ bar]:29103:1745
+; CHECK: 0: 1745
----------------
wmi wrote:
> wenlei wrote:
> > wmi wrote:
> > > wenlei wrote:
> > > > wmi wrote:
> > > > > Is it possible for us to tell one level in context is inlined or not? It will make the profile more informative. 
> > > > Yes, agree that can useful, especially for tuning purpose to see how CS inline decision differs from previous build. We wanted to add a metadata (similar to `!CFGChecksum` for pseudo-probe profile) to indicate whether a context is inlined or not. Note that in this case, it would only tell whether bar is inlined along `main:1 @ foo:3`, but not whether `foo` is inlined along `main:1.` What do you think?
> > > > 
> > > > Also to keep patch smallish, I think we can add this later separately. 
> > > > 
> > > > ```
> > > > [main:1 @ foo:3 @ bar]:29103:1745
> > > >   ...
> > > >   ...
> > > > !CFGChecksum: ...
> > > > !Flag: Inline
> > > > 
> > > > ```
> > > > 
> > > > 
> > > > Note that in this case, it would only tell whether bar is inlined along main:1 @ foo:3, but not whether foo is inlined along main:1. What do you think?
> > > 
> > > What is the main difficulty to keep the inline information for each Context level? 
> > > 
> > > > Also to keep patch smallish, I think we can add this later separately.
> > > 
> > > Sure.
> > > 
> > > ```
> > > > [main:1 @ foo:3 @ bar]:29103:1745
> > > !CFGChecksum: ...
> > > !Flag: Inline
> > > ```
> > > 
> > > Can we use some special sign to mark whether bar is inline or not, "*" for example?
> > > ```
> > > [main:1 @ foo:3 @ bar*]:29103:1745
> > > ```
> > > What is the main difficulty to keep the inline information for each Context level? 
> > 
> > Even if we only mark for leaf frame, middle frames inline decision could be found in its own leaf context (if it exists). I think it's also doable if we want to embed inline decision for each frame in a context (either with metadata or header), but since this is mostly for tuning/debugging, we're trying to keep it at minimum for now, and we can expand later if needed..
> > 
> > Now as to header vs metadata as carrier. There're couple reasons we don't do it in the header.
> > 
> > 1. We thought consistency between inline vs non-inline context for main profile and header keep things clean. In fact, CSSPGO really treats them indifferently. 
> > 2. !metadata can be mapped/converted to binary profile with general framework support. Doing it with special character in header may require special (not-so-clean) handling for text-binary conversion. 
> > 3. There's only that much we can do with special character in header, so it's not extensible if we want to encode something else. Initially we have checksum in header as well, but later move it to metadata to keep header clean, and thought it'd be good to keep all auxiliary data in metadata form.
> > 
> >  
> > We thought consistency between inline vs non-inline context for main profile and header keep things clean. In fact, CSSPGO really treats them indifferently.
> 
> Yes, I understand CSSPGO treats inline callsite or not inline callsite indifferently, and the special character showing whether a callsite is inlined is just for easy debug. 
> 
> Since it is only for debug, you just need to add it when you output the profile into text and you can strip it once for all when you read the text. That will be a standalone processing step. CSSPGO has already include a lot more useful information than current SPGO profile, I just hope the CSSPGO afdo file can show us the inline hierarchy as easy as what we current have.
> 
> I just hope the CSSPGO afdo file can show us the inline hierarchy as easy as what we current have. 

Yeah, I can see how that can make debugging a bit easier. Perhaps a stack of flags in metadata can do it too if needed. On the other hand, the info is all from dwarf, so what we are discussing is just the visualization. Visualizing inline hierarchy isn't the responsibility of a profile, but afdo happens to be able to visualize inline hierarchy in a nice way, though it's still more of a side effect of the way inline profile is represented. 

I'd argue that cleanness and consistency probably weighs more than trying to reach parity for that nice side effect (and actually even if we use * for inline frame, it's still not as nice as afdo profile's tree style inline hierarchy..) Perhaps we can leave this one open for now, and see where actual need leads us to? 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723