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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
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)
----------------
wenlei wrote:
> hoy wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > 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?) 
> > > Do you mean something like below?
> > > 
> > > 
> > > ```
> > > if (ThisContext.back().CallerName != ThatContext.back().CallerName)
> > >   return false;
> > > 
> > > if (ThisContext.back().hasLineLocation()) 
> > >    return ThisContext.back().linelocation == ThatContext.back().linelocation 
> > >    
> > > return true;
> > > ```
> > `return true` --> `return ThisContext.drop_back() == ThatContext.drop_back()`
> Yes if actually expect `ThisContext` (the prefix) to have location for its leaf (i thought the prefix's leaf won't have location as shown in your example), otherwise we could omit the location compare too. 
That's true with the current uses of this function. I'm adding an assert for that and removing the line location check. Supporting the location compare is slow, especially this function is frequently used in profile reading.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433