Skip to content

[memprof] Use std::move in ContextEdge::ContextEdge (NFC) #94687

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
Jun 7, 2024

Conversation

kazutakahirata
Copy link
Contributor

Since the constructor of ContextEdge takes ContextIds by value, we
should move it to the corresponding member variable as suggested by
clang-tidy's performance-unnecessary-value-param.

While we are at it, this patch updates a couple of callers. To avoid
the ambiguity in the evaluation order among the constructor arguments,
I'm calling computeAllocType before calling the constructor.

Since the constructor of ContextEdge takes ContextIds by value, we
should move it to the corresponding member variable as suggested by
clang-tidy's performance-unnecessary-value-param.

While we are at it, this patch updates a couple of callers.  To avoid
the ambiguity in the evaluation order among the constructor arguments,
I'm calling computeAllocType before calling the constructor.
@llvmbot
Copy link
Member

llvmbot commented Jun 6, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Kazu Hirata (kazutakahirata)

Changes

Since the constructor of ContextEdge takes ContextIds by value, we
should move it to the corresponding member variable as suggested by
clang-tidy's performance-unnecessary-value-param.

While we are at it, this patch updates a couple of callers. To avoid
the ambiguity in the evaluation order among the constructor arguments,
I'm calling computeAllocType before calling the constructor.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp (+5-5)
diff --git a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
index f033d2b0d6d01..b58b906465e56 100644
--- a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
+++ b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
@@ -402,7 +402,7 @@ class CallsiteContextGraph {
     ContextEdge(ContextNode *Callee, ContextNode *Caller, uint8_t AllocType,
                 DenseSet<uint32_t> ContextIds)
         : Callee(Callee), Caller(Caller), AllocTypes(AllocType),
-          ContextIds(ContextIds) {}
+          ContextIds(std::move(ContextIds)) {}
 
     DenseSet<uint32_t> &getContextIds() { return ContextIds; }
 
@@ -1127,15 +1127,15 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::connectNewNode(
       continue;
     }
     if (TowardsCallee) {
+      uint8_t NewAllocType = computeAllocType(NewEdgeContextIds);
       auto NewEdge = std::make_shared<ContextEdge>(
-          Edge->Callee, NewNode, computeAllocType(NewEdgeContextIds),
-          NewEdgeContextIds);
+          Edge->Callee, NewNode, NewAllocType, std::move(NewEdgeContextIds));
       NewNode->CalleeEdges.push_back(NewEdge);
       NewEdge->Callee->CallerEdges.push_back(NewEdge);
     } else {
+      uint8_t NewAllocType = computeAllocType(NewEdgeContextIds);
       auto NewEdge = std::make_shared<ContextEdge>(
-          NewNode, Edge->Caller, computeAllocType(NewEdgeContextIds),
-          NewEdgeContextIds);
+          NewNode, Edge->Caller, NewAllocType, std::move(NewEdgeContextIds));
       NewNode->CallerEdges.push_back(NewEdge);
       NewEdge->Caller->CalleeEdges.push_back(NewEdge);
     }

@kazutakahirata kazutakahirata merged commit b7d976d into llvm:main Jun 7, 2024
9 checks passed
@kazutakahirata kazutakahirata deleted the memprof_move_ContextIds branch June 7, 2024 06:49
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