[MemProf] Summary section cleanup (NFC)

[teresajohnson]

Address post-commit review comments from PR141805. Misc cleanup but the
biggest changes are moving some common utilities to new MemProfCommon
files to reduce unnecessary includes.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-pgo

Author: Teresa Johnson (teresajohnson)

Changes

Address post-commit review comments from PR141805. Misc cleanup but the
biggest changes are moving some common utilities to new MemProfCommon
files to reduce unnecessary includes.

---

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

12 Files Affected:

• (modified) llvm/include/llvm/ProfileData/IndexedMemProfData.h (+1-1)
• (added) llvm/include/llvm/ProfileData/MemProfCommon.h (+35)
• (modified) llvm/include/llvm/ProfileData/MemProfSummary.h (+1-12)
• (modified) llvm/include/llvm/ProfileData/MemProfSummaryBuilder.h (+1-1)
• (modified) llvm/lib/ProfileData/CMakeLists.txt (+1)
• (modified) llvm/lib/ProfileData/IndexedMemProfData.cpp (+2-2)
• (modified) llvm/lib/ProfileData/InstrProfWriter.cpp (+4-6)
• (added) llvm/lib/ProfileData/MemProfCommon.cpp (+81)
• (modified) llvm/lib/ProfileData/MemProfSummary.cpp (+1-65)
• (modified) llvm/lib/ProfileData/MemProfSummaryBuilder.cpp (+1)
• (modified) llvm/lib/Transforms/Instrumentation/MemProfiler.cpp (+1-1)
• (modified) llvm/unittests/ProfileData/MemProfTest.cpp (+5-7)

diff --git a/llvm/include/llvm/ProfileData/IndexedMemProfData.h b/llvm/include/llvm/ProfileData/IndexedMemProfData.h
index 304b1c4734af6..9af8755281be4 100644
--- a/llvm/include/llvm/ProfileData/IndexedMemProfData.h
+++ b/llvm/include/llvm/ProfileData/IndexedMemProfData.h
@@ -91,6 +91,6 @@ Error writeMemProf(
     ProfOStream &OS, memprof::IndexedMemProfData &MemProfData,
     memprof::IndexedVersion MemProfVersionRequested, bool MemProfFullSchema,
     std::unique_ptr<memprof::DataAccessProfData> DataAccessProfileData,
-    memprof::MemProfSummary *MemProfSum);
+    std::unique_ptr<memprof::MemProfSummary> MemProfSum);
 } // namespace llvm
 #endif
diff --git a/llvm/include/llvm/ProfileData/MemProfCommon.h b/llvm/include/llvm/ProfileData/MemProfCommon.h
new file mode 100644
index 0000000000000..ff2aeee834366
--- /dev/null
+++ b/llvm/include/llvm/ProfileData/MemProfCommon.h
@@ -0,0 +1,35 @@
+//===- MemProfCommon.h - MemProf common utilities ---------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains MemProf common utilities.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_PROFILEDATA_MEMPROFCOMMON_H
+#define LLVM_PROFILEDATA_MEMPROFCOMMON_H
+
+#include "llvm/IR/ModuleSummaryIndex.h"
+
+namespace llvm {
+namespace memprof {
+
+struct Frame;
+
+/// Return the allocation type for a given set of memory profile values.
+AllocationType getAllocType(uint64_t TotalLifetimeAccessDensity,
+                            uint64_t AllocCount, uint64_t TotalLifetime);
+
+/// Helper to generate a single hash id for a given callstack, used for emitting
+/// matching statistics and useful for uniquing such statistics across modules.
+/// Also used to dedup contexts when computing the summary.
+uint64_t computeFullStackId(ArrayRef<Frame> CallStack);
+
+} // namespace memprof
+} // namespace llvm
+
+#endif // LLVM_PROFILEDATA_MEMPROFCOMMON_H
diff --git a/llvm/include/llvm/ProfileData/MemProfSummary.h b/llvm/include/llvm/ProfileData/MemProfSummary.h
index 112c2ae9b4b6e..f1aa818d9ed77 100644
--- a/llvm/include/llvm/ProfileData/MemProfSummary.h
+++ b/llvm/include/llvm/ProfileData/MemProfSummary.h
@@ -6,29 +6,18 @@
 //
 //===----------------------------------------------------------------------===//
 //
-// This file contains MemProf summary support and related interfaces.
+// This file contains MemProf summary support.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_PROFILEDATA_MEMPROFSUMMARY_H
 #define LLVM_PROFILEDATA_MEMPROFSUMMARY_H
 
-#include "llvm/IR/ModuleSummaryIndex.h"
 #include "llvm/ProfileData/InstrProf.h"
-#include "llvm/ProfileData/MemProf.h"
 
 namespace llvm {
 namespace memprof {
 
-/// Return the allocation type for a given set of memory profile values.
-AllocationType getAllocType(uint64_t TotalLifetimeAccessDensity,
-                            uint64_t AllocCount, uint64_t TotalLifetime);
-
-/// Helper to generate a single hash id for a given callstack, used for emitting
-/// matching statistics and useful for uniquing such statistics across modules.
-/// Also used to dedup contexts when computing the summary.
-uint64_t computeFullStackId(ArrayRef<Frame> CallStack);
-
 class MemProfSummary {
 private:
   /// The number of summary fields below, which is used to enable some forwards
diff --git a/llvm/include/llvm/ProfileData/MemProfSummaryBuilder.h b/llvm/include/llvm/ProfileData/MemProfSummaryBuilder.h
index 3c615930d2210..c994f823fb945 100644
--- a/llvm/include/llvm/ProfileData/MemProfSummaryBuilder.h
+++ b/llvm/include/llvm/ProfileData/MemProfSummaryBuilder.h
@@ -26,9 +26,9 @@ class MemProfSummaryBuilder {
   // instances of the same allocations.
   DenseSet<uint64_t> Contexts;
 
+  // Helper called by the public raw and indexed profile addRecord interfaces.
   void addRecord(uint64_t, const PortableMemInfoBlock &);
 
-protected:
   uint64_t MaxColdTotalSize = 0;
   uint64_t MaxWarmTotalSize = 0;
   uint64_t MaxHotTotalSize = 0;
diff --git a/llvm/lib/ProfileData/CMakeLists.txt b/llvm/lib/ProfileData/CMakeLists.txt
index b1a0d8d707287..d26af1c3d5547 100644
--- a/llvm/lib/ProfileData/CMakeLists.txt
+++ b/llvm/lib/ProfileData/CMakeLists.txt
@@ -8,6 +8,7 @@ add_llvm_component_library(LLVMProfileData
   InstrProfWriter.cpp
   ItaniumManglingCanonicalizer.cpp
   MemProf.cpp
+  MemProfCommon.cpp
   MemProfReader.cpp
   MemProfRadixTree.cpp
   MemProfSummary.cpp
diff --git a/llvm/lib/ProfileData/IndexedMemProfData.cpp b/llvm/lib/ProfileData/IndexedMemProfData.cpp
index cbff51e3cbdc6..562ec26142065 100644
--- a/llvm/lib/ProfileData/IndexedMemProfData.cpp
+++ b/llvm/lib/ProfileData/IndexedMemProfData.cpp
@@ -314,7 +314,7 @@ Error writeMemProf(
     ProfOStream &OS, memprof::IndexedMemProfData &MemProfData,
     memprof::IndexedVersion MemProfVersionRequested, bool MemProfFullSchema,
     std::unique_ptr<memprof::DataAccessProfData> DataAccessProfileData,
-    memprof::MemProfSummary *MemProfSum) {
+    std::unique_ptr<memprof::MemProfSummary> MemProfSum) {
   switch (MemProfVersionRequested) {
   case memprof::Version2:
     return writeMemProfV2(OS, MemProfData, MemProfFullSchema);
@@ -322,7 +322,7 @@ Error writeMemProf(
     return writeMemProfV3(OS, MemProfData, MemProfFullSchema);
   case memprof::Version4:
     return writeMemProfV4(OS, MemProfData, MemProfFullSchema,
-                          std::move(DataAccessProfileData), MemProfSum);
+                          std::move(DataAccessProfileData), MemProfSum.get());
   }
 
   return make_error<InstrProfError>(
diff --git a/llvm/lib/ProfileData/InstrProfWriter.cpp b/llvm/lib/ProfileData/InstrProfWriter.cpp
index 7a4981124762c..d6e75f6f3bc9a 100644
--- a/llvm/lib/ProfileData/InstrProfWriter.cpp
+++ b/llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -618,12 +618,10 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
   if (static_cast<bool>(ProfileKind & InstrProfKind::MemProf)) {
     MemProfSectionStart = OS.tell();
 
-    // Get the finalized MemProf summary that was built when adding records.
-    auto MemProfSum = MemProfSumBuilder.getSummary();
-
-    if (auto E = writeMemProf(
-            OS, MemProfData, MemProfVersionRequested, MemProfFullSchema,
-            std::move(DataAccessProfileData), MemProfSum.get()))
+    if (auto E =
+            writeMemProf(OS, MemProfData, MemProfVersionRequested,
+                         MemProfFullSchema, std::move(DataAccessProfileData),
+                         std::move(MemProfSumBuilder.getSummary())))
       return E;
   }
 
diff --git a/llvm/lib/ProfileData/MemProfCommon.cpp b/llvm/lib/ProfileData/MemProfCommon.cpp
new file mode 100644
index 0000000000000..8c0c93123c82d
--- /dev/null
+++ b/llvm/lib/ProfileData/MemProfCommon.cpp
@@ -0,0 +1,81 @@
+//=-- MemProfCommon.cpp - MemProf common utilities ---------------=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains MemProf common utilities.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ProfileData/MemProfCommon.h"
+#include "llvm/ProfileData/MemProf.h"
+#include "llvm/Support/BLAKE3.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/HashBuilder.h"
+
+using namespace llvm;
+using namespace llvm::memprof;
+
+// Upper bound on lifetime access density (accesses per byte per lifetime sec)
+// for marking an allocation cold.
+cl::opt<float> MemProfLifetimeAccessDensityColdThreshold(
+    "memprof-lifetime-access-density-cold-threshold", cl::init(0.05),
+    cl::Hidden,
+    cl::desc("The threshold the lifetime access density (accesses per byte per "
+             "lifetime sec) must be under to consider an allocation cold"));
+
+// Lower bound on lifetime to mark an allocation cold (in addition to accesses
+// per byte per sec above). This is to avoid pessimizing short lived objects.
+cl::opt<unsigned> MemProfAveLifetimeColdThreshold(
+    "memprof-ave-lifetime-cold-threshold", cl::init(200), cl::Hidden,
+    cl::desc("The average lifetime (s) for an allocation to be considered "
+             "cold"));
+
+// Lower bound on average lifetime accesses density (total life time access
+// density / alloc count) for marking an allocation hot.
+cl::opt<unsigned> MemProfMinAveLifetimeAccessDensityHotThreshold(
+    "memprof-min-ave-lifetime-access-density-hot-threshold", cl::init(1000),
+    cl::Hidden,
+    cl::desc("The minimum TotalLifetimeAccessDensity / AllocCount for an "
+             "allocation to be considered hot"));
+
+cl::opt<bool>
+    MemProfUseHotHints("memprof-use-hot-hints", cl::init(false), cl::Hidden,
+                       cl::desc("Enable use of hot hints (only supported for "
+                                "unambigously hot allocations)"));
+
+AllocationType llvm::memprof::getAllocType(uint64_t TotalLifetimeAccessDensity,
+                                           uint64_t AllocCount,
+                                           uint64_t TotalLifetime) {
+  // The access densities are multiplied by 100 to hold 2 decimal places of
+  // precision, so need to divide by 100.
+  if (((float)TotalLifetimeAccessDensity) / AllocCount / 100 <
+          MemProfLifetimeAccessDensityColdThreshold
+      // Lifetime is expected to be in ms, so convert the threshold to ms.
+      && ((float)TotalLifetime) / AllocCount >=
+             MemProfAveLifetimeColdThreshold * 1000)
+    return AllocationType::Cold;
+
+  // The access densities are multiplied by 100 to hold 2 decimal places of
+  // precision, so need to divide by 100.
+  if (MemProfUseHotHints &&
+      ((float)TotalLifetimeAccessDensity) / AllocCount / 100 >
+          MemProfMinAveLifetimeAccessDensityHotThreshold)
+    return AllocationType::Hot;
+
+  return AllocationType::NotCold;
+}
+
+uint64_t llvm::memprof::computeFullStackId(ArrayRef<Frame> CallStack) {
+  llvm::HashBuilder<llvm::TruncatedBLAKE3<8>, llvm::endianness::little>
+      HashBuilder;
+  for (auto &F : CallStack)
+    HashBuilder.add(F.Function, F.LineOffset, F.Column);
+  llvm::BLAKE3Result<8> Hash = HashBuilder.final();
+  uint64_t Id;
+  std::memcpy(&Id, Hash.data(), sizeof(Hash));
+  return Id;
+}
diff --git a/llvm/lib/ProfileData/MemProfSummary.cpp b/llvm/lib/ProfileData/MemProfSummary.cpp
index 116eb2f805afa..f620b7a74b244 100644
--- a/llvm/lib/ProfileData/MemProfSummary.cpp
+++ b/llvm/lib/ProfileData/MemProfSummary.cpp
@@ -6,79 +6,15 @@
 //
 //===----------------------------------------------------------------------===//
 //
-// This file contains MemProf summary support and related interfaces.
+// This file contains MemProf summary support.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ProfileData/MemProfSummary.h"
-#include "llvm/Support/BLAKE3.h"
-#include "llvm/Support/CommandLine.h"
-#include "llvm/Support/HashBuilder.h"
 
 using namespace llvm;
 using namespace llvm::memprof;
 
-// Upper bound on lifetime access density (accesses per byte per lifetime sec)
-// for marking an allocation cold.
-cl::opt<float> MemProfLifetimeAccessDensityColdThreshold(
-    "memprof-lifetime-access-density-cold-threshold", cl::init(0.05),
-    cl::Hidden,
-    cl::desc("The threshold the lifetime access density (accesses per byte per "
-             "lifetime sec) must be under to consider an allocation cold"));
-
-// Lower bound on lifetime to mark an allocation cold (in addition to accesses
-// per byte per sec above). This is to avoid pessimizing short lived objects.
-cl::opt<unsigned> MemProfAveLifetimeColdThreshold(
-    "memprof-ave-lifetime-cold-threshold", cl::init(200), cl::Hidden,
-    cl::desc("The average lifetime (s) for an allocation to be considered "
-             "cold"));
-
-// Lower bound on average lifetime accesses density (total life time access
-// density / alloc count) for marking an allocation hot.
-cl::opt<unsigned> MemProfMinAveLifetimeAccessDensityHotThreshold(
-    "memprof-min-ave-lifetime-access-density-hot-threshold", cl::init(1000),
-    cl::Hidden,
-    cl::desc("The minimum TotalLifetimeAccessDensity / AllocCount for an "
-             "allocation to be considered hot"));
-
-cl::opt<bool>
-    MemProfUseHotHints("memprof-use-hot-hints", cl::init(false), cl::Hidden,
-                       cl::desc("Enable use of hot hints (only supported for "
-                                "unambigously hot allocations)"));
-
-AllocationType llvm::memprof::getAllocType(uint64_t TotalLifetimeAccessDensity,
-                                           uint64_t AllocCount,
-                                           uint64_t TotalLifetime) {
-  // The access densities are multiplied by 100 to hold 2 decimal places of
-  // precision, so need to divide by 100.
-  if (((float)TotalLifetimeAccessDensity) / AllocCount / 100 <
-          MemProfLifetimeAccessDensityColdThreshold
-      // Lifetime is expected to be in ms, so convert the threshold to ms.
-      && ((float)TotalLifetime) / AllocCount >=
-             MemProfAveLifetimeColdThreshold * 1000)
-    return AllocationType::Cold;
-
-  // The access densities are multiplied by 100 to hold 2 decimal places of
-  // precision, so need to divide by 100.
-  if (MemProfUseHotHints &&
-      ((float)TotalLifetimeAccessDensity) / AllocCount / 100 >
-          MemProfMinAveLifetimeAccessDensityHotThreshold)
-    return AllocationType::Hot;
-
-  return AllocationType::NotCold;
-}
-
-uint64_t llvm::memprof::computeFullStackId(ArrayRef<Frame> CallStack) {
-  llvm::HashBuilder<llvm::TruncatedBLAKE3<8>, llvm::endianness::little>
-      HashBuilder;
-  for (auto &F : CallStack)
-    HashBuilder.add(F.Function, F.LineOffset, F.Column);
-  llvm::BLAKE3Result<8> Hash = HashBuilder.final();
-  uint64_t Id;
-  std::memcpy(&Id, Hash.data(), sizeof(Hash));
-  return Id;
-}
-
 void MemProfSummary::printSummaryYaml(raw_ostream &OS) const {
   // For now emit as YAML comments, since they aren't read on input.
   OS << "---\n";
diff --git a/llvm/lib/ProfileData/MemProfSummaryBuilder.cpp b/llvm/lib/ProfileData/MemProfSummaryBuilder.cpp
index 591bcb6b8a3e5..2faf49ee3ec24 100644
--- a/llvm/lib/ProfileData/MemProfSummaryBuilder.cpp
+++ b/llvm/lib/ProfileData/MemProfSummaryBuilder.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ProfileData/MemProfSummaryBuilder.h"
+#include "llvm/ProfileData/MemProfCommon.h"
 
 using namespace llvm;
 using namespace llvm::memprof;
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
index deb9f8999eab7..707226bed8516 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
@@ -36,7 +36,7 @@
 #include "llvm/IR/Value.h"
 #include "llvm/ProfileData/InstrProf.h"
 #include "llvm/ProfileData/InstrProfReader.h"
-#include "llvm/ProfileData/MemProfSummary.h"
+#include "llvm/ProfileData/MemProfCommon.h"
 #include "llvm/Support/BLAKE3.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
diff --git a/llvm/unittests/ProfileData/MemProfTest.cpp b/llvm/unittests/ProfileData/MemProfTest.cpp
index ee06c2391e207..1dbafea195736 100644
--- a/llvm/unittests/ProfileData/MemProfTest.cpp
+++ b/llvm/unittests/ProfileData/MemProfTest.cpp
@@ -15,22 +15,20 @@
 #include "llvm/IR/Value.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/ProfileData/IndexedMemProfData.h"
+#include "llvm/ProfileData/MemProfCommon.h"
 #include "llvm/ProfileData/MemProfData.inc"
 #include "llvm/ProfileData/MemProfRadixTree.h"
 #include "llvm/ProfileData/MemProfReader.h"
-#include "llvm/ProfileData/MemProfSummary.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include <initializer_list>
 
-using namespace llvm;
-
-extern cl::opt<float> MemProfLifetimeAccessDensityColdThreshold;
-extern cl::opt<unsigned> MemProfAveLifetimeColdThreshold;
-extern cl::opt<unsigned> MemProfMinAveLifetimeAccessDensityHotThreshold;
-extern cl::opt<bool> MemProfUseHotHints;
+extern llvm::cl::opt<float> MemProfLifetimeAccessDensityColdThreshold;
+extern llvm::cl::opt<unsigned> MemProfAveLifetimeColdThreshold;
+extern llvm::cl::opt<unsigned> MemProfMinAveLifetimeAccessDensityHotThreshold;
+extern llvm::cl::opt<bool> MemProfUseHotHints;
 
 namespace llvm {
 namespace memprof {

[snehasish]

Thanks for the follow-up.

[snehasish]

I think we don't need the std::move since this is already a rvalue?

[teresajohnson]

done

[snehasish]

Can we move it into the v4 and update the interface there too like DataAccessProfileData?

[teresajohnson]

done

[snehasish]

lgtm