[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:575-582
+    SampleCallSiteType ThisLeaf = ThisContext.back();
+    SampleCallSiteType ThatLeaf = ThatContext.back();
+    if (!ThisLeaf.hasLineLocation()) {
+      ThisLeaf.dropLineLocation();
+      ThatLeaf.dropLineLocation();
     }
+    if (ThisLeaf != ThatLeaf)
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433