[llvm] [ProfileData] Take ArrayRef<InstrProfValueData> in addValueData (NFC) (PR #97363)
Kazu Hirata via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 10 20:03:27 PDT 2024
https://github.com/kazutakahirata updated https://github.com/llvm/llvm-project/pull/97363
>From da7e02f0021620a51c910175b872c880b99d511d Mon Sep 17 00:00:00 2001
From: Kazu Hirata <kazu at google.com>
Date: Sun, 23 Jun 2024 17:13:35 -0700
Subject: [PATCH 1/2] [ProfileData] Take ArrayRef<InstrProfValueData> in
addValueData
This patch fixes another place in ProfileData where we have a pointer
to an array of InstrProfValueData and its length separately.
addValueData is a bit unique in that it remaps incoming values in
place before adding them to ValueSites. AFAICT, no caller of
addValueData uses updated incoming values. With this patch, we add
value data to ValueSites first and then remaps values there. This
way, we can take ArrayRef<InstrProfValueData> as a parameter.
---
llvm/include/llvm/ProfileData/InstrProf.h | 2 +-
llvm/lib/ProfileData/InstrProf.cpp | 17 +++--
llvm/lib/ProfileData/InstrProfReader.cpp | 4 +-
llvm/unittests/ProfileData/InstrProfTest.cpp | 78 ++++++++++----------
4 files changed, 51 insertions(+), 50 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index 9b34cb0b651f7..2e37ade4aa69d 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -870,7 +870,7 @@ struct InstrProfRecord {
/// Add ValueData for ValueKind at value Site. We do not support adding sites
/// out of order. Site must go up from 0 one by one.
void addValueData(uint32_t ValueKind, uint32_t Site,
- InstrProfValueData *VData, uint32_t N,
+ ArrayRef<InstrProfValueData> VData,
InstrProfSymtab *SymTab);
/// Merge the counts in \p Other into this one.
diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index 93876e87f20b3..40455915f201e 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -998,18 +998,18 @@ uint64_t InstrProfRecord::remapValue(uint64_t Value, uint32_t ValueKind,
}
void InstrProfRecord::addValueData(uint32_t ValueKind, uint32_t Site,
- InstrProfValueData *VData, uint32_t N,
+ ArrayRef<InstrProfValueData> VData,
InstrProfSymtab *ValueMap) {
- for (uint32_t I = 0; I < N; I++) {
- VData[I].Value = remapValue(VData[I].Value, ValueKind, ValueMap);
- }
std::vector<InstrProfValueSiteRecord> &ValueSites =
getOrCreateValueSitesForKind(ValueKind);
assert(ValueSites.size() == Site);
- if (N == 0)
+ if (VData.empty())
ValueSites.emplace_back();
- else
- ValueSites.emplace_back(VData, VData + N);
+ else {
+ ValueSites.emplace_back(VData.begin(), VData.end());
+ for (auto &V : ValueSites.back().ValueData)
+ V.Value = remapValue(V.Value, ValueKind, ValueMap);
+ }
}
void TemporalProfTraceTy::createBPFunctionNodes(
@@ -1143,7 +1143,8 @@ void ValueProfRecord::deserializeTo(InstrProfRecord &Record,
InstrProfValueData *ValueData = getValueProfRecordValueData(this);
for (uint64_t VSite = 0; VSite < NumValueSites; ++VSite) {
uint8_t ValueDataCount = this->SiteCountArray[VSite];
- Record.addValueData(Kind, VSite, ValueData, ValueDataCount, SymTab);
+ ArrayRef<InstrProfValueData> VDs(ValueData, ValueDataCount);
+ Record.addValueData(Kind, VSite, VDs, SymTab);
ValueData += ValueDataCount;
}
}
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index e7b843362e5b3..6d078c58ac805 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -378,8 +378,8 @@ TextInstrProfReader::readValueProfileData(InstrProfRecord &Record) {
CurrentValues.push_back({Value, TakenCount});
Line++;
}
- Record.addValueData(ValueKind, S, CurrentValues.data(), NumValueData,
- nullptr);
+ assert(CurrentValues.size() == NumValueData);
+ Record.addValueData(ValueKind, S, CurrentValues, nullptr);
}
}
return success();
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index a28352057e3a6..259d6ffba2aba 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -808,13 +808,13 @@ TEST_P(InstrProfReaderWriterTest, icall_and_vtable_data_read_write) {
Record1.reserveSites(IPVK_IndirectCallTarget, 4);
InstrProfValueData VD0[] = {
{(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}, {(uint64_t)callee3, 3}};
- Record1.addValueData(IPVK_IndirectCallTarget, 0, VD0, 3, nullptr);
+ Record1.addValueData(IPVK_IndirectCallTarget, 0, VD0, nullptr);
// No value profile data at the second site.
- Record1.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
+ Record1.addValueData(IPVK_IndirectCallTarget, 1, std::nullopt, nullptr);
InstrProfValueData VD2[] = {{(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}};
- Record1.addValueData(IPVK_IndirectCallTarget, 2, VD2, 2, nullptr);
+ Record1.addValueData(IPVK_IndirectCallTarget, 2, VD2, nullptr);
InstrProfValueData VD3[] = {{(uint64_t)callee7, 1}, {(uint64_t)callee8, 2}};
- Record1.addValueData(IPVK_IndirectCallTarget, 3, VD3, 2, nullptr);
+ Record1.addValueData(IPVK_IndirectCallTarget, 3, VD3, nullptr);
}
// 2 vtable value sites.
@@ -828,8 +828,8 @@ TEST_P(InstrProfReaderWriterTest, icall_and_vtable_data_read_write) {
{getCalleeAddress(vtable1), 1},
{getCalleeAddress(vtable2), 2},
};
- Record1.addValueData(IPVK_VTableTarget, 0, VD0, 3, nullptr);
- Record1.addValueData(IPVK_VTableTarget, 1, VD2, 2, nullptr);
+ Record1.addValueData(IPVK_VTableTarget, 0, VD0, nullptr);
+ Record1.addValueData(IPVK_VTableTarget, 1, VD2, nullptr);
}
Writer.addRecord(std::move(Record1), getProfWeight(), Err);
@@ -918,7 +918,7 @@ TEST_P(MaybeSparseInstrProfTest, annotate_vp_data) {
Record.reserveSites(IPVK_IndirectCallTarget, 1);
InstrProfValueData VD0[] = {{1000, 1}, {2000, 2}, {3000, 3}, {5000, 5},
{4000, 4}, {6000, 6}};
- Record.addValueData(IPVK_IndirectCallTarget, 0, VD0, 6, nullptr);
+ Record.addValueData(IPVK_IndirectCallTarget, 0, VD0, nullptr);
Writer.addRecord(std::move(Record), Err);
auto Profile = Writer.writeBuffer();
readProfile(std::move(Profile));
@@ -1011,21 +1011,21 @@ TEST_P(MaybeSparseInstrProfTest, icall_and_vtable_data_merge) {
{uint64_t(callee2), 2},
{uint64_t(callee3), 3},
{uint64_t(callee4), 4}};
- Record11.addValueData(IPVK_IndirectCallTarget, 0, VD0, 4, nullptr);
+ Record11.addValueData(IPVK_IndirectCallTarget, 0, VD0, nullptr);
// No value profile data at the second site.
- Record11.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
+ Record11.addValueData(IPVK_IndirectCallTarget, 1, std::nullopt, nullptr);
InstrProfValueData VD2[] = {
{uint64_t(callee1), 1}, {uint64_t(callee2), 2}, {uint64_t(callee3), 3}};
- Record11.addValueData(IPVK_IndirectCallTarget, 2, VD2, 3, nullptr);
+ Record11.addValueData(IPVK_IndirectCallTarget, 2, VD2, nullptr);
InstrProfValueData VD3[] = {{uint64_t(callee7), 1}, {uint64_t(callee8), 2}};
- Record11.addValueData(IPVK_IndirectCallTarget, 3, VD3, 2, nullptr);
+ Record11.addValueData(IPVK_IndirectCallTarget, 3, VD3, nullptr);
InstrProfValueData VD4[] = {
{uint64_t(callee1), 1}, {uint64_t(callee2), 2}, {uint64_t(callee3), 3}};
- Record11.addValueData(IPVK_IndirectCallTarget, 4, VD4, 3, nullptr);
+ Record11.addValueData(IPVK_IndirectCallTarget, 4, VD4, nullptr);
}
// 3 value sites for vtables.
{
@@ -1034,53 +1034,53 @@ TEST_P(MaybeSparseInstrProfTest, icall_and_vtable_data_merge) {
{getCalleeAddress(vtable2), 2},
{getCalleeAddress(vtable3), 3},
{getCalleeAddress(vtable4), 4}};
- Record11.addValueData(IPVK_VTableTarget, 0, VD0, 4, nullptr);
+ Record11.addValueData(IPVK_VTableTarget, 0, VD0, nullptr);
InstrProfValueData VD2[] = {{getCalleeAddress(vtable1), 1},
{getCalleeAddress(vtable2), 2},
{getCalleeAddress(vtable3), 3}};
- Record11.addValueData(IPVK_VTableTarget, 1, VD2, 3, nullptr);
+ Record11.addValueData(IPVK_VTableTarget, 1, VD2, nullptr);
InstrProfValueData VD4[] = {{getCalleeAddress(vtable1), 1},
{getCalleeAddress(vtable2), 2},
{getCalleeAddress(vtable3), 3}};
- Record11.addValueData(IPVK_VTableTarget, 2, VD4, 3, nullptr);
+ Record11.addValueData(IPVK_VTableTarget, 2, VD4, nullptr);
}
// A different record for the same caller.
Record12.reserveSites(IPVK_IndirectCallTarget, 5);
InstrProfValueData VD02[] = {{uint64_t(callee2), 5}, {uint64_t(callee3), 3}};
- Record12.addValueData(IPVK_IndirectCallTarget, 0, VD02, 2, nullptr);
+ Record12.addValueData(IPVK_IndirectCallTarget, 0, VD02, nullptr);
// No value profile data at the second site.
- Record12.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
+ Record12.addValueData(IPVK_IndirectCallTarget, 1, std::nullopt, nullptr);
InstrProfValueData VD22[] = {
{uint64_t(callee2), 1}, {uint64_t(callee3), 3}, {uint64_t(callee4), 4}};
- Record12.addValueData(IPVK_IndirectCallTarget, 2, VD22, 3, nullptr);
+ Record12.addValueData(IPVK_IndirectCallTarget, 2, VD22, nullptr);
- Record12.addValueData(IPVK_IndirectCallTarget, 3, nullptr, 0, nullptr);
+ Record12.addValueData(IPVK_IndirectCallTarget, 3, std::nullopt, nullptr);
InstrProfValueData VD42[] = {
{uint64_t(callee1), 1}, {uint64_t(callee2), 2}, {uint64_t(callee3), 3}};
- Record12.addValueData(IPVK_IndirectCallTarget, 4, VD42, 3, nullptr);
+ Record12.addValueData(IPVK_IndirectCallTarget, 4, VD42, nullptr);
// 3 value sites for vtables.
{
Record12.reserveSites(IPVK_VTableTarget, 3);
InstrProfValueData VD0[] = {{getCalleeAddress(vtable2), 5},
{getCalleeAddress(vtable3), 3}};
- Record12.addValueData(IPVK_VTableTarget, 0, VD0, 2, nullptr);
+ Record12.addValueData(IPVK_VTableTarget, 0, VD0, nullptr);
InstrProfValueData VD2[] = {{getCalleeAddress(vtable2), 1},
{getCalleeAddress(vtable3), 3},
{getCalleeAddress(vtable4), 4}};
- Record12.addValueData(IPVK_VTableTarget, 1, VD2, 3, nullptr);
+ Record12.addValueData(IPVK_VTableTarget, 1, VD2, nullptr);
InstrProfValueData VD4[] = {{getCalleeAddress(vtable1), 1},
{getCalleeAddress(vtable2), 2},
{getCalleeAddress(vtable3), 3}};
- Record12.addValueData(IPVK_VTableTarget, 2, VD4, 3, nullptr);
+ Record12.addValueData(IPVK_VTableTarget, 2, VD4, nullptr);
}
Writer.addRecord(std::move(Record11), Err);
@@ -1220,7 +1220,7 @@ TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_saturation) {
NamedInstrProfRecord Record4("baz", 0x5678, {3, 4});
Record4.reserveSites(ValueKind, 1);
InstrProfValueData VD4[] = {{ProfiledValue, 1}};
- Record4.addValueData(ValueKind, 0, VD4, 1, nullptr);
+ Record4.addValueData(ValueKind, 0, VD4, nullptr);
Result = instrprof_error::success;
Writer.addRecord(std::move(Record4), Err);
ASSERT_EQ(Result, instrprof_error::success);
@@ -1229,7 +1229,7 @@ TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_saturation) {
NamedInstrProfRecord Record5("baz", 0x5678, {5, 6});
Record5.reserveSites(ValueKind, 1);
InstrProfValueData VD5[] = {{ProfiledValue, MaxValCount}};
- Record5.addValueData(ValueKind, 0, VD5, 1, nullptr);
+ Record5.addValueData(ValueKind, 0, VD5, nullptr);
Result = instrprof_error::success;
Writer.addRecord(std::move(Record5), Err);
ASSERT_EQ(Result, instrprof_error::counter_overflow);
@@ -1269,8 +1269,8 @@ TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_site_trunc) {
VD0[I].Count = 2 * I + 1000;
}
- Record11.addValueData(ValueKind, 0, VD0, 255, nullptr);
- Record11.addValueData(ValueKind, 1, nullptr, 0, nullptr);
+ Record11.addValueData(ValueKind, 0, VD0, nullptr);
+ Record11.addValueData(ValueKind, 1, std::nullopt, nullptr);
Record12.reserveSites(ValueKind, 2);
InstrProfValueData VD1[255];
@@ -1279,8 +1279,8 @@ TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_site_trunc) {
VD1[I].Count = 2 * I + 1001;
}
- Record12.addValueData(ValueKind, 0, VD1, 255, nullptr);
- Record12.addValueData(ValueKind, 1, nullptr, 0, nullptr);
+ Record12.addValueData(ValueKind, 0, VD1, nullptr);
+ Record12.addValueData(ValueKind, 1, std::nullopt, nullptr);
Writer.addRecord(std::move(Record11), Err);
// Merge profile data.
@@ -1317,23 +1317,23 @@ static void addValueProfData(InstrProfRecord &Record) {
{uint64_t(callee3), 500},
{uint64_t(callee4), 300},
{uint64_t(callee5), 100}};
- Record.addValueData(IPVK_IndirectCallTarget, 0, VD0, 5, nullptr);
+ Record.addValueData(IPVK_IndirectCallTarget, 0, VD0, nullptr);
InstrProfValueData VD1[] = {{uint64_t(callee5), 800},
{uint64_t(callee3), 1000},
{uint64_t(callee2), 2500},
{uint64_t(callee1), 1300}};
- Record.addValueData(IPVK_IndirectCallTarget, 1, VD1, 4, nullptr);
+ Record.addValueData(IPVK_IndirectCallTarget, 1, VD1, nullptr);
InstrProfValueData VD2[] = {{uint64_t(callee6), 800},
{uint64_t(callee3), 1000},
{uint64_t(callee4), 5500}};
- Record.addValueData(IPVK_IndirectCallTarget, 2, VD2, 3, nullptr);
+ Record.addValueData(IPVK_IndirectCallTarget, 2, VD2, nullptr);
InstrProfValueData VD3[] = {{uint64_t(callee2), 1800},
{uint64_t(callee3), 2000}};
- Record.addValueData(IPVK_IndirectCallTarget, 3, VD3, 2, nullptr);
- Record.addValueData(IPVK_IndirectCallTarget, 4, nullptr, 0, nullptr);
+ Record.addValueData(IPVK_IndirectCallTarget, 3, VD3, nullptr);
+ Record.addValueData(IPVK_IndirectCallTarget, 4, std::nullopt, nullptr);
InstrProfValueData VD5[] = {{uint64_t(callee7), 1234},
{uint64_t(callee8), 5678}};
- Record.addValueData(IPVK_IndirectCallTarget, 5, VD5, 2, nullptr);
+ Record.addValueData(IPVK_IndirectCallTarget, 5, VD5, nullptr);
}
// Add test data for vtables
@@ -1355,10 +1355,10 @@ static void addValueProfData(InstrProfRecord &Record) {
};
InstrProfValueData VD3[] = {{getCalleeAddress(vtable2), 1800},
{getCalleeAddress(vtable3), 2000}};
- Record.addValueData(IPVK_VTableTarget, 0, VD0, 5, nullptr);
- Record.addValueData(IPVK_VTableTarget, 1, VD1, 4, nullptr);
- Record.addValueData(IPVK_VTableTarget, 2, VD2, 3, nullptr);
- Record.addValueData(IPVK_VTableTarget, 3, VD3, 2, nullptr);
+ Record.addValueData(IPVK_VTableTarget, 0, VD0, nullptr);
+ Record.addValueData(IPVK_VTableTarget, 1, VD1, nullptr);
+ Record.addValueData(IPVK_VTableTarget, 2, VD2, nullptr);
+ Record.addValueData(IPVK_VTableTarget, 3, VD3, nullptr);
}
}
>From 46c5f2e140edb341eead5a3ec54b74998deb4280 Mon Sep 17 00:00:00 2001
From: Kazu Hirata <kazu at google.com>
Date: Wed, 10 Jul 2024 19:34:17 -0700
Subject: [PATCH 2/2] Remap values in a separate vector first.
---
llvm/include/llvm/ProfileData/InstrProf.h | 5 ++---
llvm/lib/ProfileData/InstrProf.cpp | 18 +++++++++++-------
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index 2e37ade4aa69d..d449091078c93 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -794,9 +794,8 @@ struct InstrProfValueSiteRecord {
std::vector<InstrProfValueData> ValueData;
InstrProfValueSiteRecord() = default;
- template <class InputIterator>
- InstrProfValueSiteRecord(InputIterator F, InputIterator L)
- : ValueData(F, L) {}
+ InstrProfValueSiteRecord(std::vector<InstrProfValueData> &&VD)
+ : ValueData(VD) {}
/// Sort ValueData ascending by Value
void sortByTargetValues() {
diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index 40455915f201e..e38855c92b1a3 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -1000,16 +1000,20 @@ uint64_t InstrProfRecord::remapValue(uint64_t Value, uint32_t ValueKind,
void InstrProfRecord::addValueData(uint32_t ValueKind, uint32_t Site,
ArrayRef<InstrProfValueData> VData,
InstrProfSymtab *ValueMap) {
+ // Remap values.
+ std::vector<InstrProfValueData> RemappedVD;
+ RemappedVD.reserve(VData.size());
+ for (const auto &V : VData) {
+ uint64_t NewValue = remapValue(V.Value, ValueKind, ValueMap);
+ RemappedVD.push_back({NewValue, V.Count});
+ }
+
std::vector<InstrProfValueSiteRecord> &ValueSites =
getOrCreateValueSitesForKind(ValueKind);
assert(ValueSites.size() == Site);
- if (VData.empty())
- ValueSites.emplace_back();
- else {
- ValueSites.emplace_back(VData.begin(), VData.end());
- for (auto &V : ValueSites.back().ValueData)
- V.Value = remapValue(V.Value, ValueKind, ValueMap);
- }
+
+ // Add a new value site with remapped value profiling data.
+ ValueSites.emplace_back(std::move(RemappedVD));
}
void TemporalProfTraceTy::createBPFunctionNodes(
More information about the llvm-commits
mailing list