[PATCH] D140908: [MemProf] Context disambiguation cloning pass [patch 1/3]

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks for the comments. I haven't had a chance to address them yet, but most of the suggestions make sense I think. I added just a few responses here.



================
Comment at: llvm/include/llvm/ADT/SetOperations.h:60
+template <class S1Ty, class S2Ty>
+S1Ty set_intersection(const S1Ty &S1, const S2Ty &S2) {
+   if (S1.size() < S2.size())
----------------
snehasish wrote:
> davidxl wrote:
> > Keep the name of the interface 'set_intersect'. To allow overloading, make the return a reference parameter.
> If we make the return a reference (eg. `S1Ty&`) this will break since we are returning a temporary. Returning `const S1Ty&` would work due to reference lifetime extension but constrains what we can do with the result.
I think it should stay with a value return, not a reference, also to be consistent with other functions in this file that return the result set by value. Also, I do want the name to be different (also to be consistent with what is done elsewhere in this file - see e.g. set_subtract vs set_difference) - I think it is clearer to have a different name for the function that returns the result vs updating the first parameter.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:992
+  CallsitesTy &mutableCallsites() {
+    assert(Callsites);
+    return *Callsites;
----------------
snehasish wrote:
> The behaviour of the const and mutable version of the callsites function differ. In `callsites` if its null it returns an empty array ref, for mutableCallsites it may assert (or segfault on an opt build). Should we avoid the assert by allocating storage instead?
I'd prefer not to allocate, based on where and how this is used. Since it is returning a reference, I can't have it return an empty vector like callsites().


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:805
+    if (TowardsCallee) {
+      auto *NewEdge = new ContextEdge(Edge->Callee, NewNode,
+                                      computeAllocType(NewEdgeContextIds),
----------------
snehasish wrote:
> Can we use shared_ptr for the edges so that we don't have to manually track ownership?
> 
> (also mentioned in the other patch)
Yes we could. I have some concern about the additional overhead and whether it is worth it given that tracking these didn't end up being too difficult. I'll collect some measurements, it might be small given the overhead of all the rest of the graph memory.


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:1212
+
+IndexCallsiteContextGraph::IndexCallsiteContextGraph(
+    ModuleSummaryIndex &Index,
----------------
snehasish wrote:
> Can we split this patch to only implement the basic version (non-ThinLTO)? 
Will do for commit - as we discussed offline, at this point I may do so after the review which is already in progress here.


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:1596
+template <typename DerivedCCG, typename FuncTy, typename CallTy>
+void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::exportToDot(
+    std::string Path) const {
----------------
snehasish wrote:
> It would be good to use llvm/Support/GraphWriter.h to export as dotGraph instead of re-implementing things here. You may need to use DOTGraphTraits.h too to implement some things such as tooltips which are used here.
> 
> This is probably a significant change but it would reduce the amount of code we have to maintain in this file.
Thanks for that pointer, will do. I copied the approach used by ModuleSummaryIndex, which seems like it should eventually be migrated to using the GraphWriter as well!


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:1
+//===-- PGHOContextDisambiguation.cpp - Disambiguate contexts -------------===//
+//
----------------
snehasish wrote:
> So far we've used memprof to refer to all of the prior work, I would strongly prefer we continue using the same to make it easy to discover all the related code by just searching for a single term. This is also useful for listing all the options related to memprof which start with the "memprof-" prefix. Wdyt? 
ack - I was wondering the same thing myself, will do a rename.


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:122
+  /// Represents a function clone via FuncTy pointer and clone number pair.
+  struct FuncInfo : public std::pair<FuncTy *, unsigned /*Clone number*/> {
+    using Base = std::pair<FuncTy *, unsigned>;
----------------
snehasish wrote:
> I guess inheriting from std::pair provides equality operators etc? Is there any other reason to derive from std::pair as compared to just defining the members inline.
> 
> nit: struct FuncInfo final (same for CallInfo below)
Yes, it is for inheriting the operators, rather than redefining them. This same trick is used elsewhere in llvm.


================
Comment at: llvm/test/Transforms/PGHOContextDisambiguation/basic.ll:29
+;;
+;; Code compiled with -mllvm -memprof-min-lifetime-cold-threshold=5 so that the
+;; memory freed after sleep(10) results in cold lifetimes.
----------------
davidxl wrote:
> why is the option called '-lifetime-cold-'? should it be '-lifetime-short-'?
This is the minimum lifetime required in order to mark the context at cold when we do profile matching.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140908