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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added a comment.

Still need to look into `assignFunctions` but before I do I'm wondering if this patch can be split into two with `applyImport` and `assignFunctions` in separate patches. What do you think?



================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:135
 
+  bool assignFunctions();
+
----------------
Add a function comment?


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2336
+    std::map<FuncInfo, std::map<CallInfo, CallInfo>> FuncClonesToCallMap;
+    for (auto Call : FuncEntry.second) {
+      ContextNode *Node = getNodeForInst(Call);
----------------
auto& Call to avoid a copy?


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2764
+
+  auto IsMemProfFunc = [](const Function &F) {
+    return F.getName().contains(".memprof.");
----------------
Is `IsMemProfClone` more accurate? 


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2765
+  auto IsMemProfFunc = [](const Function &F) {
+    return F.getName().contains(".memprof.");
+  };
----------------
Can we move this to a common const string somewhere? It would be good to use the same constant when assigning the name for the clone in `getMemProfFuncName`.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2788
+    unsigned NumClonesCreated = 0;
+    auto CloneFuncIfNeeded = [&](unsigned NumClones) {
+      // We should at least have version 0 which is the original copy.
----------------
Can we move this functor to a static (or private) helper method? 

I think we can return the ValueToValueMap vector as a return value and the number of clones created is implicit from the size of the vector.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2808
+      for (unsigned I = 1; I < NumClones; I++) {
+        VMaps.emplace_back(new ValueToValueMapTy());
+        auto *NewF = CloneFunction(&F, *VMaps.back());
----------------
Can we use make_unique instead of new here? Looking at new made me wonder if there was a leak.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2855
+
+    // Locate the summary for F. This is complicated by the fact that it might
+    // have been internalized or promoted.
----------------
Can we move L2855-L2904 to a helper which returns a function summary (or nullptr)?


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2937
+            MaxAllocVersionsThinBackend = 1;
+        }
+
----------------
Can we skip the rest of the loop body here since MemProfMD will be false below? I think we just need to duplicate the metadata stripping code.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2949
+            assert(MIBIter != AllocNode.MIBs.end());
+            auto &MIB = *(MIBIter++);
+            auto StackIdIndexIter = MIB.StackIdIndices.begin();
----------------
Just increment MIBIter here and below we can use `MIBIter->StackIdIndices` (since I don't see other uses of MIB). We already use this spelling in the assert below.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2973
+          // Perform cloning if not yet done.
+          CloneFuncIfNeeded(AllocNode.Versions.size());
+
----------------
Add a comment for the param /*NumClones=*/


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:3016
+            else
+              CBClone = cast<CallBase>((*VMaps[J - 1])[CB]);
+            CBClone->addFnAttr(A);
----------------
IIUC we index with J-1 since the loop which populates the VMap starts from I=1  on L2807. This seems a little brittle. Perhaps add a comment?


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:3152
+        errorOrToExpected(MemoryBuffer::getFile(MemProfImportSummary)));
+    if (Expected<std::unique_ptr<ModuleSummaryIndex>> SummaryOrErr =
+            getModuleSummaryIndex(*ReadSummaryFile)) {
----------------
I think this will silently skip the if condition on error since the bool operation for the condition will set the error as checked [1]. Wondering if this is intended since the previous line exits on error.

[1] https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/Error.h#L234


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141077