Skip to content

[MemProf] Remove unnecessary data structure (NFC) #107643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

teresajohnson
Copy link
Contributor

Recent change #106623 added the CallToFunc map, but I subsequently
realized the same information is already available for the calls being
examined in the StackIdToMatchingCalls map we're iterating through.

Recent change llvm#106623 added the CallToFunc map, but I subsequently
realized the same information is already available for the calls being
examined in the StackIdToMatchingCalls map we're iterating through.
@llvmbot
Copy link
Member

llvmbot commented Sep 6, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Teresa Johnson (teresajohnson)

Changes

Recent change #106623 added the CallToFunc map, but I subsequently
realized the same information is already available for the calls being
examined in the StackIdToMatchingCalls map we're iterating through.


Full diff: https://github.com/llvm/llvm-project/pull/107643.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp (+3-15)
diff --git a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
index 52def8f21312de..f8c150a675e645 100644
--- a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
+++ b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
@@ -464,9 +464,6 @@ class CallsiteContextGraph {
   /// iteration.
   MapVector<FuncTy *, std::vector<CallInfo>> FuncToCallsWithMetadata;
 
-  /// Records the function each call is located in.
-  DenseMap<CallInfo, const FuncTy *> CallToFunc;
-
   /// Map from callsite node to the enclosing caller function.
   std::map<const ContextNode *, const FuncTy *> NodeToCallingFunc;
 
@@ -1575,15 +1572,13 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::updateStackNodes() {
           continue;
       }
 
-      const FuncTy *CallFunc = CallToFunc[Call];
-
       // If the prior call had the same stack ids this map would not be empty.
       // Check if we already have a call that "matches" because it is located
       // in the same function.
-      if (FuncToCallMap.contains(CallFunc)) {
+      if (FuncToCallMap.contains(Func)) {
         // Record the matching call found for this call, and skip it. We
         // will subsequently combine it into the same node.
-        CallToMatchingCall[Call] = FuncToCallMap[CallFunc];
+        CallToMatchingCall[Call] = FuncToCallMap[Func];
         continue;
       }
 
@@ -1623,7 +1618,7 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::updateStackNodes() {
         // Record the call with its function, so we can locate it the next time
         // we find a call from this function when processing the calls with the
         // same stack ids.
-        FuncToCallMap[CallFunc] = Call;
+        FuncToCallMap[Func] = Call;
     }
   }
 
@@ -1741,7 +1736,6 @@ ModuleCallsiteContextGraph::ModuleCallsiteContextGraph(
           continue;
         if (auto *MemProfMD = I.getMetadata(LLVMContext::MD_memprof)) {
           CallsWithMetadata.push_back(&I);
-          CallToFunc[&I] = &F;
           auto *AllocNode = addAllocNode(&I, &F);
           auto *CallsiteMD = I.getMetadata(LLVMContext::MD_callsite);
           assert(CallsiteMD);
@@ -1765,7 +1759,6 @@ ModuleCallsiteContextGraph::ModuleCallsiteContextGraph(
         // For callsite metadata, add to list for this function for later use.
         else if (I.getMetadata(LLVMContext::MD_callsite)) {
           CallsWithMetadata.push_back(&I);
-          CallToFunc[&I] = &F;
         }
       }
     }
@@ -1823,7 +1816,6 @@ IndexCallsiteContextGraph::IndexCallsiteContextGraph(
             continue;
           IndexCall AllocCall(&AN);
           CallsWithMetadata.push_back(AllocCall);
-          CallToFunc[AllocCall] = FS;
           auto *AllocNode = addAllocNode(AllocCall, FS);
           // Pass an empty CallStack to the CallsiteContext (second)
           // parameter, since for ThinLTO we already collapsed out the inlined
@@ -1858,7 +1850,6 @@ IndexCallsiteContextGraph::IndexCallsiteContextGraph(
         for (auto &SN : FS->mutableCallsites()) {
           IndexCall StackNodeCall(&SN);
           CallsWithMetadata.push_back(StackNodeCall);
-          CallToFunc[StackNodeCall] = FS;
         }
 
       if (!CallsWithMetadata.empty())
@@ -1942,9 +1933,6 @@ void CallsiteContextGraph<DerivedCCG, FuncTy,
           // want to do this during iteration over that map, so save the calls
           // that need updated entries.
           NewCallToNode.push_back({ThisCall, Node});
-          // We should only have shared this node between calls from the same
-          // function.
-          assert(NodeToCallingFunc[Node] == CallToFunc[Node->Call]);
         }
         break;
       }

Copy link
Contributor

@snehasish snehasish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, is there a noticeable change to memory consumption?

@teresajohnson
Copy link
Contributor Author

lgtm, is there a noticeable change to memory consumption?

No, maybe a small <1% reduction. But I saw a few percent time reduction.

@teresajohnson teresajohnson merged commit e46f03b into llvm:main Sep 9, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants