[ProfileData] Add getValueArrayForSite #95335

kazutakahirata · 2024-06-13T00:03:48Z

Without this patch, a typical traversal over the value data looks
like:

uint32_t NV = Func.getNumValueDataForSite(VK, S);
std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, S);
for (uint32_t V = 0; V < NV; V++)
Do something with VD[V].Value and/or VD[V].Count;

This patch adds getValueArrayForSite, which returns
ArrayRef, so we can do:

for (const auto &V : Func.getValueArrayForSite(VK, S))
Do something with V.Value and/or V.Count;

I'm planning to migrate the existing uses of getValueForSite to
getValueArrayForSite in follow-up patches and remove getValueForSite
and getNumValueDataForSite.

llvmbot · 2024-06-13T00:04:21Z

@llvm/pr-subscribers-pgo

Author: Kazu Hirata (kazutakahirata)

Changes

Without this patch, a typical traversal over the value data looks
like:

uint32_t NV = Func.getNumValueDataForSite(VK, S);
std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, S);
for (uint32_t V = 0; V < NV; V++)
Do something with VD[V].Value and/or VD[V].Count;

With this patch, getValueForSite returns ArrayRef<InstrProfValueData>,
so we can do:

for (const auto &V : Func.getValueForSite(VK, S))
Do something with V.Value and/or V.Count;

To accommodate the migration, this patch renames the existing
getValueForSite and its uses to getValueForSiteLegacy. This patch
silently switches to the new signature for unit tests that don't
require any change like:

auto VD0 = R->getValueForSite(IPVK_VTableTarget, 0);
EXPECT_EQ(VD0[0].Value, getCalleeAddress(vtable2));
EXPECT_EQ(VD0[0].Count, 7U);

where we receive the return value with auto and reference the array
contents.

Everything else uses getValueForSiteLegacy for now. I'm planning to
migrate to the new signature and remove getValueForSiteLegacy and
getNumValueDataForSite in follow-up patches.

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

5 Files Affected:

(modified) llvm/include/llvm/ProfileData/InstrProf.h (+18-7)
(modified) llvm/lib/ProfileData/InstrProf.cpp (+4-3)
(modified) llvm/lib/ProfileData/InstrProfWriter.cpp (+4-2)
(modified) llvm/tools/llvm-profdata/llvm-profdata.cpp (+2-1)
(modified) llvm/unittests/ProfileData/InstrProfTest.cpp (+19-18)

diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index 817ad9550f652..45e29d8394856 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -870,13 +870,17 @@ struct InstrProfRecord {
                                          uint32_t Site) const;
 
   /// Return the array of profiled values at \p Site.
-  inline std::unique_ptr<InstrProfValueData[]>
+  inline ArrayRef<InstrProfValueData>
   getValueForSite(uint32_t ValueKind, uint32_t Site) const;
 
+  /// Return the array of profiled values at \p Site.
+  inline std::unique_ptr<InstrProfValueData[]>
+  getValueForSiteLegacy(uint32_t ValueKind, uint32_t Site) const;
+
   /// Get the target value/counts of kind \p ValueKind collected at site
   /// \p Site and store the result in array \p Dest.
-  inline void getValueForSite(InstrProfValueData Dest[], uint32_t ValueKind,
-                              uint32_t Site) const;
+  inline void getValueForSiteLegacy(InstrProfValueData Dest[],
+                                    uint32_t ValueKind, uint32_t Site) const;
 
   /// Reserve space for NumValueSites sites.
   inline void reserveSites(uint32_t ValueKind, uint32_t NumValueSites);
@@ -1060,20 +1064,27 @@ uint32_t InstrProfRecord::getNumValueDataForSite(uint32_t ValueKind,
   return getValueSitesForKind(ValueKind)[Site].ValueData.size();
 }
 
-std::unique_ptr<InstrProfValueData[]>
+ArrayRef<InstrProfValueData>
 InstrProfRecord::getValueForSite(uint32_t ValueKind, uint32_t Site) const {
+  return getValueSitesForKind(ValueKind)[Site].ValueData;
+}
+
+std::unique_ptr<InstrProfValueData[]>
+InstrProfRecord::getValueForSiteLegacy(uint32_t ValueKind,
+                                       uint32_t Site) const {
   uint32_t N = getNumValueDataForSite(ValueKind, Site);
   if (N == 0)
     return std::unique_ptr<InstrProfValueData[]>(nullptr);
 
   auto VD = std::make_unique<InstrProfValueData[]>(N);
-  getValueForSite(VD.get(), ValueKind, Site);
+  getValueForSiteLegacy(VD.get(), ValueKind, Site);
 
   return VD;
 }
 
-void InstrProfRecord::getValueForSite(InstrProfValueData Dest[],
-                                      uint32_t ValueKind, uint32_t Site) const {
+void InstrProfRecord::getValueForSiteLegacy(InstrProfValueData Dest[],
+                                            uint32_t ValueKind,
+                                            uint32_t Site) const {
   uint32_t I = 0;
   for (auto V : getValueSitesForKind(ValueKind)[Site].ValueData) {
     Dest[I].Value = V.Value;
diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index 184e2c86d6584..a77f932195203 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -732,7 +732,7 @@ void InstrProfRecord::accumulateCounts(CountSumOrPercent &Sum) const {
     uint32_t NumValueSites = getNumValueSites(VK);
     for (size_t I = 0; I < NumValueSites; ++I) {
       uint32_t NV = getNumValueDataForSite(VK, I);
-      std::unique_ptr<InstrProfValueData[]> VD = getValueForSite(VK, I);
+      std::unique_ptr<InstrProfValueData[]> VD = getValueForSiteLegacy(VK, I);
       for (uint32_t V = 0; V < NV; V++)
         KindSum += VD[V].Count;
     }
@@ -1095,7 +1095,8 @@ uint32_t getNumValueDataForSiteInstrProf(const void *R, uint32_t VK,
 
 void getValueForSiteInstrProf(const void *R, InstrProfValueData *Dst,
                               uint32_t K, uint32_t S) {
-  reinterpret_cast<const InstrProfRecord *>(R)->getValueForSite(Dst, K, S);
+  reinterpret_cast<const InstrProfRecord *>(R)->getValueForSiteLegacy(Dst, K,
+                                                                      S);
 }
 
 ValueProfData *allocValueProfDataInstrProf(size_t TotalSizeInBytes) {
@@ -1279,7 +1280,7 @@ void annotateValueSite(Module &M, Instruction &Inst,
     return;
 
   std::unique_ptr<InstrProfValueData[]> VD =
-      InstrProfR.getValueForSite(ValueKind, SiteIdx);
+      InstrProfR.getValueForSiteLegacy(ValueKind, SiteIdx);
 
   ArrayRef<InstrProfValueData> VDs(VD.get(), NV);
   uint64_t Sum = 0;
diff --git a/llvm/lib/ProfileData/InstrProfWriter.cpp b/llvm/lib/ProfileData/InstrProfWriter.cpp
index 1a9add109a360..7263950273394 100644
--- a/llvm/lib/ProfileData/InstrProfWriter.cpp
+++ b/llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -1040,7 +1040,8 @@ Error InstrProfWriter::validateRecord(const InstrProfRecord &Func) {
       continue;
     for (uint32_t S = 0; S < NS; S++) {
       uint32_t ND = Func.getNumValueDataForSite(VK, S);
-      std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, S);
+      std::unique_ptr<InstrProfValueData[]> VD =
+          Func.getValueForSiteLegacy(VK, S);
       DenseSet<uint64_t> SeenValues;
       for (uint32_t I = 0; I < ND; I++)
         if ((VK != IPVK_IndirectCallTarget && VK != IPVK_VTableTarget) &&
@@ -1090,7 +1091,8 @@ void InstrProfWriter::writeRecordInText(StringRef Name, uint64_t Hash,
     for (uint32_t S = 0; S < NS; S++) {
       uint32_t ND = Func.getNumValueDataForSite(VK, S);
       OS << ND << "\n";
-      std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, S);
+      std::unique_ptr<InstrProfValueData[]> VD =
+          Func.getValueForSiteLegacy(VK, S);
       for (uint32_t I = 0; I < ND; I++) {
         if (VK == IPVK_IndirectCallTarget || VK == IPVK_VTableTarget)
           OS << Symtab.getFuncOrVarNameIfDefined(VD[I].Value) << ":"
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index fae6d1e989ab5..0c67bace60561 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -2696,7 +2696,8 @@ static void traverseAllValueSites(const InstrProfRecord &Func, uint32_t VK,
   Stats.TotalNumValueSites += NS;
   for (size_t I = 0; I < NS; ++I) {
     uint32_t NV = Func.getNumValueDataForSite(VK, I);
-    std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, I);
+    std::unique_ptr<InstrProfValueData[]> VD =
+        Func.getValueForSiteLegacy(VK, I);
     Stats.TotalNumValues += NV;
     if (NV) {
       Stats.TotalNumValueSitesWithValueProfile++;
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index 8acb0fa0c717a..8573a8d00c4e4 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -1135,7 +1135,7 @@ TEST_P(MaybeSparseInstrProfTest, icall_and_vtable_data_merge) {
     EXPECT_STREQ((const char *)VD[3].Value, "callee1");
     EXPECT_EQ(VD[3].Count, 1U);
 
-    auto VD_2(R->getValueForSite(IPVK_IndirectCallTarget, 2));
+    auto VD_2(R->getValueForSiteLegacy(IPVK_IndirectCallTarget, 2));
     EXPECT_STREQ((const char *)VD_2[0].Value, "callee3");
     EXPECT_EQ(VD_2[0].Count, 6U);
     EXPECT_STREQ((const char *)VD_2[1].Value, "callee4");
@@ -1145,13 +1145,13 @@ TEST_P(MaybeSparseInstrProfTest, icall_and_vtable_data_merge) {
     EXPECT_STREQ((const char *)VD_2[3].Value, "callee1");
     EXPECT_EQ(VD_2[3].Count, 1U);
 
-    auto VD_3(R->getValueForSite(IPVK_IndirectCallTarget, 3));
+    auto VD_3(R->getValueForSiteLegacy(IPVK_IndirectCallTarget, 3));
     EXPECT_STREQ((const char *)VD_3[0].Value, "callee8");
     EXPECT_EQ(VD_3[0].Count, 2U);
     EXPECT_STREQ((const char *)VD_3[1].Value, "callee7");
     EXPECT_EQ(VD_3[1].Count, 1U);
 
-    auto VD_4(R->getValueForSite(IPVK_IndirectCallTarget, 4));
+    auto VD_4(R->getValueForSiteLegacy(IPVK_IndirectCallTarget, 4));
     EXPECT_STREQ((const char *)VD_4[0].Value, "callee3");
     EXPECT_EQ(VD_4[0].Count, 6U);
     EXPECT_STREQ((const char *)VD_4[1].Value, "callee2");
@@ -1256,7 +1256,7 @@ TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_saturation) {
   ASSERT_TRUE(bool(ReadRecord2));
   ASSERT_EQ(1U, ReadRecord2->getNumValueSites(ValueKind));
   std::unique_ptr<InstrProfValueData[]> VD =
-      ReadRecord2->getValueForSite(ValueKind, 0);
+      ReadRecord2->getValueForSiteLegacy(ValueKind, 0);
   EXPECT_EQ(ProfiledValue, VD[0].Value);
   EXPECT_EQ(MaxValCount, VD[0].Count);
 }
@@ -1300,7 +1300,8 @@ TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_site_trunc) {
 
   Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
   ASSERT_THAT_ERROR(R.takeError(), Succeeded());
-  std::unique_ptr<InstrProfValueData[]> VD(R->getValueForSite(ValueKind, 0));
+  std::unique_ptr<InstrProfValueData[]> VD(
+      R->getValueForSiteLegacy(ValueKind, 0));
   ASSERT_EQ(2U, R->getNumValueSites(ValueKind));
   EXPECT_EQ(255U, R->getNumValueDataForSite(ValueKind, 0));
   for (unsigned I = 0; I < 255; I++) {
@@ -1394,7 +1395,7 @@ TEST(ValueProfileReadWriteTest, value_prof_data_read_write) {
   };
 
   std::unique_ptr<InstrProfValueData[]> VD_0(
-      Record.getValueForSite(IPVK_IndirectCallTarget, 0));
+      Record.getValueForSiteLegacy(IPVK_IndirectCallTarget, 0));
   llvm::sort(&VD_0[0], &VD_0[5], Cmp);
   EXPECT_STREQ((const char *)VD_0[0].Value, "callee2");
   EXPECT_EQ(1000U, VD_0[0].Count);
@@ -1408,7 +1409,7 @@ TEST(ValueProfileReadWriteTest, value_prof_data_read_write) {
   EXPECT_EQ(100U, VD_0[4].Count);
 
   std::unique_ptr<InstrProfValueData[]> VD_1(
-      Record.getValueForSite(IPVK_IndirectCallTarget, 1));
+      Record.getValueForSiteLegacy(IPVK_IndirectCallTarget, 1));
   llvm::sort(&VD_1[0], &VD_1[4], Cmp);
   EXPECT_STREQ((const char *)VD_1[0].Value, "callee2");
   EXPECT_EQ(VD_1[0].Count, 2500U);
@@ -1420,7 +1421,7 @@ TEST(ValueProfileReadWriteTest, value_prof_data_read_write) {
   EXPECT_EQ(VD_1[3].Count, 800U);
 
   std::unique_ptr<InstrProfValueData[]> VD_2(
-      Record.getValueForSite(IPVK_IndirectCallTarget, 2));
+      Record.getValueForSiteLegacy(IPVK_IndirectCallTarget, 2));
   llvm::sort(&VD_2[0], &VD_2[3], Cmp);
   EXPECT_STREQ((const char *)VD_2[0].Value, "callee4");
   EXPECT_EQ(VD_2[0].Count, 5500U);
@@ -1430,7 +1431,7 @@ TEST(ValueProfileReadWriteTest, value_prof_data_read_write) {
   EXPECT_EQ(VD_2[2].Count, 800U);
 
   std::unique_ptr<InstrProfValueData[]> VD_3(
-      Record.getValueForSite(IPVK_IndirectCallTarget, 3));
+      Record.getValueForSiteLegacy(IPVK_IndirectCallTarget, 3));
   llvm::sort(&VD_3[0], &VD_3[2], Cmp);
   EXPECT_STREQ((const char *)VD_3[0].Value, "callee3");
   EXPECT_EQ(VD_3[0].Count, 2000U);
@@ -1443,7 +1444,7 @@ TEST(ValueProfileReadWriteTest, value_prof_data_read_write) {
   ASSERT_EQ(Record.getNumValueDataForSite(IPVK_VTableTarget, 2), 3U);
   ASSERT_EQ(Record.getNumValueDataForSite(IPVK_VTableTarget, 3), 2U);
 
-  auto VD0(Record.getValueForSite(IPVK_VTableTarget, 0));
+  auto VD0(Record.getValueForSiteLegacy(IPVK_VTableTarget, 0));
   llvm::sort(&VD0[0], &VD0[5], Cmp);
   EXPECT_EQ(VD0[0].Value, getCalleeAddress(vtable2));
   EXPECT_EQ(VD0[0].Count, 1000U);
@@ -1456,7 +1457,7 @@ TEST(ValueProfileReadWriteTest, value_prof_data_read_write) {
   EXPECT_EQ(VD0[4].Value, getCalleeAddress(vtable5));
   EXPECT_EQ(VD0[4].Count, 100U);
 
-  auto VD1(Record.getValueForSite(IPVK_VTableTarget, 1));
+  auto VD1(Record.getValueForSiteLegacy(IPVK_VTableTarget, 1));
   llvm::sort(&VD1[0], &VD1[4], Cmp);
   EXPECT_EQ(VD1[0].Value, getCalleeAddress(vtable2));
   EXPECT_EQ(VD1[0].Count, 2500U);
@@ -1467,7 +1468,7 @@ TEST(ValueProfileReadWriteTest, value_prof_data_read_write) {
   EXPECT_EQ(VD1[3].Value, getCalleeAddress(vtable5));
   EXPECT_EQ(VD1[3].Count, 800U);
 
-  auto VD2(Record.getValueForSite(IPVK_VTableTarget, 2));
+  auto VD2(Record.getValueForSiteLegacy(IPVK_VTableTarget, 2));
   llvm::sort(&VD2[0], &VD2[3], Cmp);
   EXPECT_EQ(VD2[0].Value, getCalleeAddress(vtable4));
   EXPECT_EQ(VD2[0].Count, 5500U);
@@ -1476,7 +1477,7 @@ TEST(ValueProfileReadWriteTest, value_prof_data_read_write) {
   EXPECT_EQ(VD2[2].Value, getCalleeAddress(vtable6));
   EXPECT_EQ(VD2[2].Count, 800U);
 
-  auto VD3(Record.getValueForSite(IPVK_VTableTarget, 3));
+  auto VD3(Record.getValueForSiteLegacy(IPVK_VTableTarget, 3));
   llvm::sort(&VD3[0], &VD3[2], Cmp);
   EXPECT_EQ(VD3[0].Value, getCalleeAddress(vtable3));
   EXPECT_EQ(VD3[0].Count, 2000U);
@@ -1538,7 +1539,7 @@ TEST(ValueProfileReadWriteTest, symtab_mapping) {
   auto Cmp = [](const InstrProfValueData &VD1, const InstrProfValueData &VD2) {
     return VD1.Count > VD2.Count;
   };
-  auto VD_0(Record.getValueForSite(IPVK_IndirectCallTarget, 0));
+  auto VD_0(Record.getValueForSiteLegacy(IPVK_IndirectCallTarget, 0));
   llvm::sort(&VD_0[0], &VD_0[5], Cmp);
   ASSERT_EQ(VD_0[0].Value, 0x2000ULL);
   ASSERT_EQ(VD_0[0].Count, 1000U);
@@ -1555,7 +1556,7 @@ TEST(ValueProfileReadWriteTest, symtab_mapping) {
 
   {
     // The first vtable site.
-    auto VD(Record.getValueForSite(IPVK_VTableTarget, 0));
+    auto VD(Record.getValueForSiteLegacy(IPVK_VTableTarget, 0));
     ASSERT_EQ(Record.getNumValueDataForSite(IPVK_VTableTarget, 0), 5U);
     llvm::sort(&VD[0], &VD[5], Cmp);
     EXPECT_EQ(VD[0].Count, 1000U);
@@ -1574,7 +1575,7 @@ TEST(ValueProfileReadWriteTest, symtab_mapping) {
 
   {
     // The second vtable site.
-    auto VD(Record.getValueForSite(IPVK_VTableTarget, 1));
+    auto VD(Record.getValueForSiteLegacy(IPVK_VTableTarget, 1));
     ASSERT_EQ(Record.getNumValueDataForSite(IPVK_VTableTarget, 1), 4U);
     llvm::sort(&VD[0], &VD[4], Cmp);
     EXPECT_EQ(VD[0].Value, MD5Hash("vtable2"));
@@ -1591,7 +1592,7 @@ TEST(ValueProfileReadWriteTest, symtab_mapping) {
 
   {
     // The third vtable site.
-    auto VD(Record.getValueForSite(IPVK_VTableTarget, 2));
+    auto VD(Record.getValueForSiteLegacy(IPVK_VTableTarget, 2));
     ASSERT_EQ(Record.getNumValueDataForSite(IPVK_VTableTarget, 2), 3U);
     llvm::sort(&VD[0], &VD[3], Cmp);
     EXPECT_EQ(VD[0].Count, 5500U);
@@ -1605,7 +1606,7 @@ TEST(ValueProfileReadWriteTest, symtab_mapping) {
 
   {
     // The fourth vtable site.
-    auto VD(Record.getValueForSite(IPVK_VTableTarget, 3));
+    auto VD(Record.getValueForSiteLegacy(IPVK_VTableTarget, 3));
     ASSERT_EQ(Record.getNumValueDataForSite(IPVK_VTableTarget, 3), 2U);
     llvm::sort(&VD[0], &VD[2], Cmp);
     EXPECT_EQ(VD[0].Count, 2000U);

github-actions · 2024-06-13T00:06:47Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…Data> Without this patch, a typical traversal over the value data looks like: uint32_t NV = Func.getNumValueDataForSite(VK, S); std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, S); for (uint32_t V = 0; V < NV; V++) Do something with VD[V].Value and/or VD[V].Count; With this patch, getValueForSite returns ArrayRef<InstrProfValueData>, so we can do: for (const auto &V : Func.getValueForSite(VK, S)) Do something with V.Value and/or V.Count; To accommodate the migration, this patch renames the existing getValueForSite and its uses to getValueForSiteLegacy. This patch silently switches to the new signature for unit tests that don't require any change like: auto VD0 = R->getValueForSite(IPVK_VTableTarget, 0); EXPECT_EQ(VD0[0].Value, getCalleeAddress(vtable2)); EXPECT_EQ(VD0[0].Count, 7U); where we receive the return value with auto and reference the array contents. Everything else uses getValueForSiteLegacy for now. I'm planning to migrate to the new signature and remove getValueForSiteLegacy and getNumValueDataForSite in follow-up patches.

mingmingl-llvm

thanks!

mingmingl-llvm · 2024-06-13T15:54:10Z

llvm/tools/llvm-profdata/llvm-profdata.cpp

@@ -2696,7 +2696,8 @@ static void traverseAllValueSites(const InstrProfRecord &Func, uint32_t VK,
  Stats.TotalNumValueSites += NS;
  for (size_t I = 0; I < NS; ++I) {
    uint32_t NV = Func.getNumValueDataForSite(VK, I);
-    std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, I);
+    std::unique_ptr<InstrProfValueData[]> VD =


nit: auto VD = ...

ref: 'Local variable type deduction' in https://google.github.io/styleguide/cppguide.html

Thanks! The latest iteration does not touch this part of the code. I'll keep auto VD in mind when I migrate the code in question to getValueArrayForSite in follow-up patches.

mingmingl-llvm · 2024-06-13T15:54:52Z

llvm/lib/ProfileData/InstrProfWriter.cpp

@@ -1090,7 +1091,8 @@ void InstrProfWriter::writeRecordInText(StringRef Name, uint64_t Hash,
    for (uint32_t S = 0; S < NS; S++) {
      uint32_t ND = Func.getNumValueDataForSite(VK, S);
      OS << ND << "\n";
-      std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, S);
+      std::unique_ptr<InstrProfValueData[]> VD =


nit: auto VD = ...

Likewise, the latest iteration does not touch this part of the code. I'll keep auto VD in mind when I migrate the code in question to getValueArrayForSite in follow-up patches.

mingmingl-llvm · 2024-06-13T17:21:24Z

llvm/include/llvm/ProfileData/InstrProf.h

+  /// Return the array of profiled values at \p Site.
+  inline ArrayRef<InstrProfValueData> getValueForSite(uint32_t ValueKind,
+                                                      uint32_t Site) const;
+
  /// Return the array of profiled values at \p Site.


The new interface returns an ArrayRef and the deprecated interface allows caller to take ownership of the array.

I wonder if there are existing call sites where explicit ownership is preferred over a reference (e.g., easy to reason about lifecycle, etc).

If yes I think we may need both.

I wouldn't worry about the lifetime of profile data. The profile data stays available at least during the entire duration of a pass.

david-xl

I suggest leaving the old interface name unchanged and make the new interface getValueArrayForSite.

kazutakahirata · 2024-06-13T18:04:03Z

I suggest leaving the old interface name unchanged and make the new interface getValueArrayForSite.

Fixed in the latest iteration and updated the title and commit comment accordingly.

PTAL again. Thanks!

Without this patch, a typical traversal over the value data looks like: uint32_t NV = Func.getNumValueDataForSite(VK, S); std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, S); for (uint32_t V = 0; V < NV; V++) Do something with VD[V].Value and/or VD[V].Count; This patch adds getValueArrayForSite, which returns ArrayRef<InstrProfValueData>, so we can do: for (const auto &V : Func.getValueArrayForSite(VK, S)) Do something with V.Value and/or V.Count; I'm planning to migrate the existing uses of getValueForSite to getValueArrayForSite in follow-up patches and remove getValueForSite and getNumValueDataForSite.

kazutakahirata requested review from mingmingl-llvm and david-xl June 13, 2024 00:03

llvmbot added the PGO Profile Guided Optimizations label Jun 13, 2024

kazutakahirata added 2 commits June 13, 2024 08:33

Fix formatting.

0ce2216

kazutakahirata force-pushed the cleanup_valueprof_getValueForSite branch from 78d153b to 0ce2216 Compare June 13, 2024 15:33

mingmingl-llvm reviewed Jun 13, 2024

View reviewed changes

david-xl reviewed Jun 13, 2024

View reviewed changes

Use getValueArrayForSite instead of getValueForSite.

5a34074

kazutakahirata changed the title ~~[ProfileData] Teach getValueForSite to return ArrayRef<InstrProfValueData>~~ [ProfileData] Add getValueArrayForSite Jun 13, 2024

david-xl approved these changes Jun 13, 2024

View reviewed changes

Fix formatting.

84cec0d

kazutakahirata merged commit 4b493e3 into llvm:main Jun 13, 2024
4 of 6 checks passed

kazutakahirata deleted the cleanup_valueprof_getValueForSite branch June 13, 2024 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ProfileData] Add getValueArrayForSite #95335

[ProfileData] Add getValueArrayForSite #95335

Uh oh!

kazutakahirata commented Jun 13, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jun 13, 2024

Uh oh!

github-actions bot commented Jun 13, 2024 •

edited

Loading

Uh oh!

mingmingl-llvm left a comment

Uh oh!

mingmingl-llvm Jun 13, 2024

Uh oh!

mingmingl-llvm Jun 13, 2024

Uh oh!

kazutakahirata Jun 13, 2024

Uh oh!

mingmingl-llvm Jun 13, 2024

Uh oh!

kazutakahirata Jun 13, 2024

Uh oh!

mingmingl-llvm Jun 13, 2024

Uh oh!

mingmingl-llvm Jun 13, 2024

Uh oh!

kazutakahirata Jun 13, 2024

Uh oh!

david-xl left a comment

Uh oh!

kazutakahirata commented Jun 13, 2024

Uh oh!

Uh oh!

Uh oh!

[ProfileData] Add getValueArrayForSite #95335

[ProfileData] Add getValueArrayForSite #95335

Uh oh!

Conversation

kazutakahirata commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 13, 2024

Uh oh!

github-actions bot commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mingmingl-llvm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-xl left a comment

Choose a reason for hiding this comment

Uh oh!

kazutakahirata commented Jun 13, 2024

Uh oh!

Uh oh!

Uh oh!

kazutakahirata commented Jun 13, 2024 •

edited

Loading

github-actions bot commented Jun 13, 2024 •

edited

Loading