Skip to content
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

[memprof] Add another constructor to IndexedAllocationInfo (NFC) #116684

Conversation

kazutakahirata
Copy link
Contributor

This patch adds another constructor to IndexedAllocationInfo that is
identical to the existing constructor except that the new one leaves
the CallStack field empty.

I'm planning to remove MemProf format Version 1. Then we will migrate
the users of the existing constructor to the new one as nobody will be
using the CallStack field anymore.

Adding the new constructor now allows us to migrate a few existing
users of the old constructor even before we remove the CallStack
field. In turn, that simplifies the patch to actually remove the
field.

This patch adds another constructor to IndexedAllocationInfo that is
identical to the existing constructor except that the new one leaves
the CallStack field empty.

I'm planning to remove MemProf format Version 1.  Then we will migrate
the users of the existing constructor to the new one as nobody will be
using the CallStack field anymore.

Adding the new constructor now allows us to migrate a few existing
users of the old constructor even before we remove the CallStack
field.  In turn, that simplifies the patch to actually remove the
field.
@llvmbot llvmbot added the PGO Profile Guided Optimizations label Nov 18, 2024
@llvmbot
Copy link

llvmbot commented Nov 18, 2024

@llvm/pr-subscribers-pgo

Author: Kazu Hirata (kazutakahirata)

Changes

This patch adds another constructor to IndexedAllocationInfo that is
identical to the existing constructor except that the new one leaves
the CallStack field empty.

I'm planning to remove MemProf format Version 1. Then we will migrate
the users of the existing constructor to the new one as nobody will be
using the CallStack field anymore.

Adding the new constructor now allows us to migrate a few existing
users of the old constructor even before we remove the CallStack
field. In turn, that simplifies the patch to actually remove the
field.


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

3 Files Affected:

  • (modified) llvm/include/llvm/ProfileData/MemProf.h (+5)
  • (modified) llvm/unittests/ProfileData/InstrProfTest.cpp (+1-2)
  • (modified) llvm/unittests/ProfileData/MemProfTest.cpp (+4-6)
diff --git a/llvm/include/llvm/ProfileData/MemProf.h b/llvm/include/llvm/ProfileData/MemProf.h
index 6a29e3df9629bc..9415e554bcc0ab 100644
--- a/llvm/include/llvm/ProfileData/MemProf.h
+++ b/llvm/include/llvm/ProfileData/MemProf.h
@@ -354,10 +354,15 @@ struct IndexedAllocationInfo {
   PortableMemInfoBlock Info;
 
   IndexedAllocationInfo() = default;
+  // This constructor is soft deprecated.  It will be removed once we remove all
+  // users of the CallStack field.
   IndexedAllocationInfo(ArrayRef<FrameId> CS, CallStackId CSId,
                         const MemInfoBlock &MB,
                         const MemProfSchema &Schema = getFullSchema())
       : CallStack(CS), CSId(CSId), Info(MB, Schema) {}
+  IndexedAllocationInfo(CallStackId CSId, const MemInfoBlock &MB,
+                        const MemProfSchema &Schema = getFullSchema())
+      : CSId(CSId), Info(MB, Schema) {}
 
   // Returns the size in bytes when this allocation info struct is serialized.
   size_t serializedSize(const MemProfSchema &Schema,
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index b9f244104c65cf..5a313aa4182a54 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -415,8 +415,7 @@ makeRecordV2(std::initializer_list<::llvm::memprof::CallStackId> AllocFrames,
   for (const auto &CSId : AllocFrames)
     // We don't populate IndexedAllocationInfo::CallStack because we use it only
     // in Version1.
-    MR.AllocSites.emplace_back(::llvm::SmallVector<memprof::FrameId>(), CSId,
-                               Block, Schema);
+    MR.AllocSites.emplace_back(CSId, Block, Schema);
   for (const auto &CSId : CallSiteFrames)
     MR.CallSiteIds.push_back(CSId);
   return MR;
diff --git a/llvm/unittests/ProfileData/MemProfTest.cpp b/llvm/unittests/ProfileData/MemProfTest.cpp
index c90669811e60ae..5097dbdd6c3915 100644
--- a/llvm/unittests/ProfileData/MemProfTest.cpp
+++ b/llvm/unittests/ProfileData/MemProfTest.cpp
@@ -315,7 +315,7 @@ TEST(MemProf, RecordSerializationRoundTripVerion2) {
   IndexedMemProfRecord Record;
   for (const auto &CSId : CallStackIds) {
     // Use the same info block for both allocation sites.
-    Record.AllocSites.emplace_back(llvm::SmallVector<FrameId>(), CSId, Info);
+    Record.AllocSites.emplace_back(CSId, Info);
   }
   Record.CallSiteIds.assign(CallSiteIds);
 
@@ -346,8 +346,7 @@ TEST(MemProf, RecordSerializationRoundTripVersion2HotColdSchema) {
   IndexedMemProfRecord Record;
   for (const auto &CSId : CallStackIds) {
     // Use the same info block for both allocation sites.
-    Record.AllocSites.emplace_back(llvm::SmallVector<FrameId>(), CSId, Info,
-                                   Schema);
+    Record.AllocSites.emplace_back(CSId, Info, Schema);
   }
   Record.CallSiteIds.assign(CallSiteIds);
 
@@ -510,7 +509,6 @@ TEST(MemProf, BaseMemProfReaderWithCSIdMap) {
   Block.AllocCount = 1U, Block.TotalAccessDensity = 4,
   Block.TotalLifetime = 200001;
   FakeRecord.AllocSites.emplace_back(
-      /*CS=*/llvm::SmallVector<FrameId>(),
       /*CSId=*/llvm::memprof::hashCallStack(CallStack),
       /*MB=*/Block);
   ProfData.insert({F1.hash(), FakeRecord});
@@ -610,7 +608,7 @@ MemInfoBlock makePartialMIB() {
 TEST(MemProf, MissingCallStackId) {
   // Use a non-existent CallStackId to trigger a mapping error in
   // toMemProfRecord.
-  llvm::memprof::IndexedAllocationInfo AI({}, 0xdeadbeefU, makePartialMIB(),
+  llvm::memprof::IndexedAllocationInfo AI(0xdeadbeefU, makePartialMIB(),
                                           llvm::memprof::getHotColdSchema());
 
   IndexedMemProfRecord IndexedMR;
@@ -633,7 +631,7 @@ TEST(MemProf, MissingCallStackId) {
 }
 
 TEST(MemProf, MissingFrameId) {
-  llvm::memprof::IndexedAllocationInfo AI({}, 0x222, makePartialMIB(),
+  llvm::memprof::IndexedAllocationInfo AI(0x222, makePartialMIB(),
                                           llvm::memprof::getHotColdSchema());
 
   IndexedMemProfRecord IndexedMR;

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

llvm/include/llvm/ProfileData/MemProf.h Show resolved Hide resolved
@kazutakahirata kazutakahirata merged commit a4e1a3d into llvm:main Nov 18, 2024
10 checks passed
@kazutakahirata kazutakahirata deleted the memprof_IndexedAllocationInfo_CallStack branch November 18, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants