[ProfileData] Take ArrayRef<InstrProfValueData> in addValueData (NFC) #97363
+57
−53
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kazutakahirata
changed the title
|
@llvm/pr-subscribers-pgo Author: Kazu Hirata (kazutakahirata) ChangesThis patch fixes another place in ProfileData where we have a pointer addValueData is a bit unique in that it remaps incoming values in Full diff: https://github.com/llvm/llvm-project/pull/97363.diff 4 Files Affected:
diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index 50e6f1d3b9b1f..059380daee238 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -880,7 +880,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 9dbaa2ca0f020..7501fb1e99c9d 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -999,18 +999,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(
@@ -1144,7 +1144,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 e18ce5d373d1c..af30afe3d254b 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);
}
}
|
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.
kazutakahirata
force-pushed
the
cleanup_ProfileData_addValueData
branch
from
1df6561
to
da7e02f
Compare
minglotus-6
reviewed
Jul 10, 2024
|
Please take a look. Thanks! |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
…llvm#97363) 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 as a parameter.