[PATCH] D108433: [CSSPGO] split context string - compiler changes

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:449-457
 // Sample context for FunctionSamples. It consists of the calling context,
 // the function name and context state. Internally sample context is represented
 // using StringRef, which is also the input for constructing a `SampleContext`.
 // It can accept and represent both full context string as well as context-less
 // function name.
 // Example of full context string (note the wrapping `[]`):
 //    `[main:3 @ _Z5funcAi:1 @ _Z8funcLeafi]`
----------------
hoy wrote:
> wenlei wrote:
> > This needs update. 
> > 
> > One thing I was looking for a clarification from comments somewhere but didn't find it: whether context-less profile would have a non-empty FullContext which only contains leaf? From the code it seems answer is no? But then this is not consistent since FullContext of CS profile contains leaf frame.
> > 
> > Or would it be possible to remove leaf frame in FullContext for CS profile too? leaf doesn't need call site location either. 
> The new context representation should be consistent with the old one for CS profile, i.e, it can contain a leaf frame only. The sample reader creates such leaf-only context. For non-CS profile, the context string will be empty.
> 
> Excluding leaf frame from `FullContext` should also work and it is cleaner by design. But the current way is more natural to implement, for example the `IsPrefixOf` operation, and some operations in llvm-profgen that appends the leaf probe into an existing calling context. Also the constructor takes in an arrayRef of the underlying vector, which, if separated, will need some special handling in the reader.
makes sense, thanks for clarification. though it is indeed inconsistent with non-CS profile in the sense that FullContext doesn't contain leaf for non-CS, but contain leaf for CS profile. 


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:575-582
+    SampleCallSiteType ThisLeaf = ThisContext.back();
+    SampleCallSiteType ThatLeaf = ThatContext.back();
+    if (!ThisLeaf.hasLineLocation()) {
+      ThisLeaf.dropLineLocation();
+      ThatLeaf.dropLineLocation();
     }
+    if (ThisLeaf != ThatLeaf)
----------------
hoy wrote:
> wenlei wrote:
> > This sequence of copy, edit and compare could be simplified?
> > 
> > ```
> > if (ThisContext.back().CallerName != ThatContext.back().CallerName)
> >   return false;
> > ```
> > 
> > Even better, would it be possible to canonicalize context, so location of leaf frame is always (0, 0)?
> The API can be used to compare `A:1 @ B` with `A:1 @ B:2 @ C`, where we have to drop the line number of `B:2`.
> 
> The location of leaf frame was (0,0) previously, now I'm changing it to (-1,0). But the `that` context can still have a valid line number like `B:2` where we have to drop the line number for comparision.
Ok, if we have to ignore the location part of B, can we simplified the copy, edit and compare sequence as suggested above still? 

(maybe i'm missing something, but it looks to me that you answered the 2nd question only?) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433