[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:26
 #include "llvm/Support/Allocator.h"
+#include "llvm/Support/DJB.h"
 #include "llvm/Support/Debug.h"
----------------
wenlei wrote:
> I don't see DJB hash used anywhere, remove as well?
Removed.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:423
+
+  std::string str(bool OutputLineLocation) const {
+    std::ostringstream OContextStr;
----------------
wenlei wrote:
> I think as long as we don't use 0 line offset to indicate no line info, it's fine. 
> 
> Actually if we use CallSiteLocation to represent leaf, this really isn't a call site (call site always has a line location), but rather a frame. The comment is also inaccurate: "Represent a callsite with caller function name and line location".
> 
> We have `typedef std::pair<std::string, LineLocation> FrameLocation;` in llvm-profgen, once we have string buffers owning the underlying strings, that FrameLocation can be merged too.  
> 
> Btw, I hope we can unify string converter to be `toString`. Right now we have CallSiteLocation::str(), SampleContext::getContextString() and SampleContext::toString. 
> 
> SampleContext::toString properly dispatches for CS and non-CS case, but SampleContext::getContextString() doesn't. How about merge the two and let toString take a boolean? 
Right `CallSiteLocation` is really for a callsite, we just take advantage of it to represent a leaf frame in `SampleContext`, and we intentionally ignore the line location for leaf frame. This isn't ideal, but easier to implement. Alternatively, the `LineLocation` field can be made an `Optional` field, but that adds the complexity most of the time we don't need.

The `FrameLocation` type is being replaced by D108437.

I'm renaming `CallSiteLocation::str()` as `toString`. `SampleContext::getContextString()` is a static function that works on an `ArrayRef` object. This is mostly for the sake of intermediate contexts (without leaf probe) in llvm-profgen. `SampleContext::toString` is not supposed to work with intermediate contexts. I can move `getContextString` out of `SampleContext` if it is  confusing here.




================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:573
+  bool operator>(const SampleContext &That) const {
+    return (*this != That) && !(*this < That);
+  }
----------------
wlei wrote:
> is this more heavy operator than direct implementation of `>`(like `<`).
> 
> say if two contexts are very long and only the last frame is different.
> 
> it will take 2 * n for (*this != That) plus  !(*this < That), rather than n.
Good point. I just removed the `>` operator which is only used by sorting profiles for dumping. The sorting will use `<` instead.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:307-309
+      ContextTrieNode *FromNode = getContextFor(Context);
+      if (FromNode == Node)
+        continue;
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > Curious what triggered this order change?
> > The context of an inlined or merged node may be out-of-sync with its tree path so `getContextFor` will AV in that case. This can happen when functions on a tree path are not processed in top-down order, due to recursions.
> Could you share a concrete example for such case? I was expecting context to be in-sync with its position in trie, because when we promote a node in trie, we also adjust its context accordingly. Does it also happen before this change? 
Unfortunately I don't seem to find that extreme case after adjusting the context promotion order, i.e, with a comparer. On the other hand, if we are not going to do anything for an inlined or merged context, we can bail out earlier without taking time to find its context node?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433