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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added a comment.

Still reviewing, here are some initial comments.



================
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())
----------------
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.


================
Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:131
   CallStackIterator beginAfterSharedPrefix(CallStack &Other);
+  uint64_t last() const;
 
----------------
nit: call this back() to be consistent with iterator method names?


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:318
 
+inline raw_ostream &operator<<(raw_ostream &OS, const CallsiteInfo &SNI) {
+  OS << "Callee: " << SNI.Callee;
----------------
davidxl wrote:
> dumping utilities can be split out. Similarly for new interfaces like 'last', or set_subtract .
+1


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:992
+  CallsitesTy &mutableCallsites() {
+    assert(Callsites);
+    return *Callsites;
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:872
+  DenseSet<ContextNode *> NodesToDelete;
+  auto ProcessNode = [&](ContextNode *Node,
+                         DenseSet<const ContextNode *> &Visited,
----------------
This lambda is doing a lot of work, can me move this out to its own method? It's a little hard to read right now because of the size and doing so would make it a little easier to read. 


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:896
+
+    auto Calls = StackIdToMatchingCalls[Node->OrigStackOrAllocId];
+    // Handle the simple case first. A single call with a single stack id.
----------------
auto& to avoid a copy?


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:1102
+    return Base.str();
+  return (Base + ".pgho." + std::to_string(CloneNo)).str();
+}
----------------
Twine(CloneNo) should incur fewer conversions.
"pgho" suffix would change if you consider the comment about pgho/memprof above.


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:1212
+
+IndexCallsiteContextGraph::IndexCallsiteContextGraph(
+    ModuleSummaryIndex &Index,
----------------
Can we split this patch to only implement the basic version (non-ThinLTO)? 


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:1
+//===-- PGHOContextDisambiguation.cpp - Disambiguate contexts -------------===//
+//
----------------
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? 


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:59
+static cl::opt<bool> DumpCCG("pgho-dump-ccg", cl::init(false), cl::Hidden,
+                             cl::desc("Dump CCG to stdout after each stage."));
+
----------------
nit: Expand CCG to CallingContextGraph here and below.


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:68
+
+inline bool hasSingleAllocType(uint8_t AllocTypes) {
+  switch (AllocTypes) {
----------------
Can we move this to MemoryProfileInfo.h and use it here as well as MemoryProfileInfo.cpp where it exists as a static function?


================
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>;
----------------
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)


================
Comment at: llvm/lib/Transforms/IPO/PGHOContextDisambiguation.cpp:142
+    void print(raw_ostream &OS) const {
+      if (!(bool)*this) {
+        assert(!cloneNo());
----------------
nit: IMO `operator bool()` is less cryptic than `(bool)*this`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140908