[clang] [llvm] [InstrProf] Factor out getRecord() and use NamedInstrProfRecord (PR #145417)
Ellis Hoag via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 24 08:18:23 PDT 2025
https://github.com/ellishg updated https://github.com/llvm/llvm-project/pull/145417
>From 5ba3eab61982f9989c665091c672283b610b539d Mon Sep 17 00:00:00 2001
From: Ellis Hoag <ellishoag at meta.com>
Date: Mon, 23 Jun 2025 15:11:01 -0700
Subject: [PATCH 1/3] [InstrProf] Factor out getRecord and use NamedRecord
---
clang/lib/CodeGen/CodeGenPGO.cpp | 3 +-
.../llvm/ProfileData/InstrProfReader.h | 2 +-
llvm/lib/ProfileData/InstrProfReader.cpp | 6 +--
.../Instrumentation/PGOInstrumentation.cpp | 45 ++++++++++---------
llvm/unittests/ProfileData/InstrProfTest.cpp | 20 ++++-----
5 files changed, 37 insertions(+), 39 deletions(-)
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index a80bebbb4cf41..98b30e084b18b 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -1423,8 +1423,7 @@ void CodeGenPGO::loadRegionCounts(llvm::IndexedInstrProfReader *PGOReader,
bool IsInMainFile) {
CGM.getPGOStats().addVisited(IsInMainFile);
RegionCounts.clear();
- llvm::Expected<llvm::InstrProfRecord> RecordExpected =
- PGOReader->getInstrProfRecord(FuncName, FunctionHash);
+ auto RecordExpected = PGOReader->getInstrProfRecord(FuncName, FunctionHash);
if (auto E = RecordExpected.takeError()) {
auto IPE = std::get<0>(llvm::InstrProfError::take(std::move(E)));
if (IPE == llvm::instrprof_error::unknown_function)
diff --git a/llvm/include/llvm/ProfileData/InstrProfReader.h b/llvm/include/llvm/ProfileData/InstrProfReader.h
index 3b12505d3326c..deb5cd17d8fd9 100644
--- a/llvm/include/llvm/ProfileData/InstrProfReader.h
+++ b/llvm/include/llvm/ProfileData/InstrProfReader.h
@@ -824,7 +824,7 @@ class LLVM_ABI IndexedInstrProfReader : public InstrProfReader {
/// functions, MismatchedFuncSum returns the maximum. If \c FuncName is not
/// found, try to lookup \c DeprecatedFuncName to handle profiles built by
/// older compilers.
- Expected<InstrProfRecord>
+ Expected<NamedInstrProfRecord>
getInstrProfRecord(StringRef FuncName, uint64_t FuncHash,
StringRef DeprecatedFuncName = "",
uint64_t *MismatchedFuncSum = nullptr);
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index ab109cd5b13a7..5c7b9e0544030 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -1369,7 +1369,7 @@ InstrProfSymtab &IndexedInstrProfReader::getSymtab() {
return *Symtab;
}
-Expected<InstrProfRecord> IndexedInstrProfReader::getInstrProfRecord(
+Expected<NamedInstrProfRecord> IndexedInstrProfReader::getInstrProfRecord(
StringRef FuncName, uint64_t FuncHash, StringRef DeprecatedFuncName,
uint64_t *MismatchedFuncSum) {
ArrayRef<NamedInstrProfRecord> Data;
@@ -1572,7 +1572,7 @@ memprof::AllMemProfData IndexedMemProfReader::getAllMemProfData() const {
Error IndexedInstrProfReader::getFunctionCounts(StringRef FuncName,
uint64_t FuncHash,
std::vector<uint64_t> &Counts) {
- Expected<InstrProfRecord> Record = getInstrProfRecord(FuncName, FuncHash);
+ auto Record = getInstrProfRecord(FuncName, FuncHash);
if (Error E = Record.takeError())
return error(std::move(E));
@@ -1583,7 +1583,7 @@ Error IndexedInstrProfReader::getFunctionCounts(StringRef FuncName,
Error IndexedInstrProfReader::getFunctionBitmap(StringRef FuncName,
uint64_t FuncHash,
BitVector &Bitmap) {
- Expected<InstrProfRecord> Record = getInstrProfRecord(FuncName, FuncHash);
+ auto Record = getInstrProfRecord(FuncName, FuncHash);
if (Error E = Record.takeError())
return error(std::move(E));
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index 9c92a7feb6e61..eddbf73bb6c1c 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1176,15 +1176,18 @@ class PGOUseFunc {
void handleInstrProfError(Error Err, uint64_t MismatchedFuncSum);
+ // Get the profile record and handle errors if necessary.
+ bool getRecord(IndexedInstrProfReader *PGOReader);
+
// Read counts for the instrumented BB from profile.
- bool readCounters(IndexedInstrProfReader *PGOReader, bool &AllZeros,
+ bool readCounters(bool &AllZeros,
InstrProfRecord::CountPseudoKind &PseudoKind);
// Populate the counts for all BBs.
void populateCounters();
// Set block coverage based on profile coverage values.
- void populateCoverage(IndexedInstrProfReader *PGOReader);
+ void populateCoverage();
// Set the branch weights based on the count values.
void setBranchWeights();
@@ -1441,14 +1444,9 @@ void PGOUseFunc::handleInstrProfError(Error Err, uint64_t MismatchedFuncSum) {
});
}
-// Read the profile from ProfileFileName and assign the value to the
-// instrumented BB and the edges. This function also updates ProgramMaxCount.
-// Return true if the profile are successfully read, and false on errors.
-bool PGOUseFunc::readCounters(IndexedInstrProfReader *PGOReader, bool &AllZeros,
- InstrProfRecord::CountPseudoKind &PseudoKind) {
- auto &Ctx = M->getContext();
+bool PGOUseFunc::getRecord(IndexedInstrProfReader *PGOReader) {
uint64_t MismatchedFuncSum = 0;
- Expected<InstrProfRecord> Result = PGOReader->getInstrProfRecord(
+ auto Result = PGOReader->getInstrProfRecord(
FuncInfo.FuncName, FuncInfo.FunctionHash, FuncInfo.DeprecatedFuncName,
&MismatchedFuncSum);
if (Error E = Result.takeError()) {
@@ -1456,6 +1454,16 @@ bool PGOUseFunc::readCounters(IndexedInstrProfReader *PGOReader, bool &AllZeros,
return false;
}
ProfileRecord = std::move(Result.get());
+ ProgramMaxCount = PGOReader->getMaximumFunctionCount(IsCS);
+ return true;
+}
+
+// Read the profile from ProfileFileName and assign the value to the
+// instrumented BB and the edges. This function also updates ProgramMaxCount.
+// Return true if the profile are successfully read, and false on errors.
+bool PGOUseFunc::readCounters(bool &AllZeros,
+ InstrProfRecord::CountPseudoKind &PseudoKind) {
+ auto &Ctx = M->getContext();
PseudoKind = ProfileRecord.getCountPseudoKind();
if (PseudoKind != InstrProfRecord::NotPseudo) {
return true;
@@ -1488,22 +1496,13 @@ bool PGOUseFunc::readCounters(IndexedInstrProfReader *PGOReader, bool &AllZeros,
DS_Warning));
return false;
}
- ProgramMaxCount = PGOReader->getMaximumFunctionCount(IsCS);
return true;
}
-void PGOUseFunc::populateCoverage(IndexedInstrProfReader *PGOReader) {
- uint64_t MismatchedFuncSum = 0;
- Expected<InstrProfRecord> Result = PGOReader->getInstrProfRecord(
- FuncInfo.FuncName, FuncInfo.FunctionHash, FuncInfo.DeprecatedFuncName,
- &MismatchedFuncSum);
- if (auto Err = Result.takeError()) {
- handleInstrProfError(std::move(Err), MismatchedFuncSum);
- return;
- }
+void PGOUseFunc::populateCoverage() {
IsCS ? NumOfCSPGOFunc++ : NumOfPGOFunc++;
- std::vector<uint64_t> &CountsFromProfile = Result.get().Counts;
+ ArrayRef<uint64_t> CountsFromProfile = ProfileRecord.Counts;
DenseMap<const BasicBlock *, bool> Coverage;
unsigned Index = 0;
for (auto &BB : F)
@@ -2243,8 +2242,10 @@ static bool annotateAllFunctions(
PGOUseFunc Func(F, &M, TLI, ComdatMembers, BPI, BFI, LI, PSI, IsCS,
InstrumentFuncEntry, InstrumentLoopEntries,
HasSingleByteCoverage);
+ if (!Func.getRecord(PGOReader.get()))
+ continue;
if (HasSingleByteCoverage) {
- Func.populateCoverage(PGOReader.get());
+ Func.populateCoverage();
continue;
}
// When PseudoKind is set to a vaule other than InstrProfRecord::NotPseudo,
@@ -2253,7 +2254,7 @@ static bool annotateAllFunctions(
// attribute and drop all the profile counters.
InstrProfRecord::CountPseudoKind PseudoKind = InstrProfRecord::NotPseudo;
bool AllZeros = false;
- if (!Func.readCounters(PGOReader.get(), AllZeros, PseudoKind))
+ if (!Func.readCounters(AllZeros, PseudoKind))
continue;
if (AllZeros) {
F.setEntryCount(ProfileCount(0, Function::PCT_Real));
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index dcdacb903791d..6467695355be8 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -135,7 +135,7 @@ TEST_P(MaybeSparseInstrProfTest, get_instr_prof_record) {
auto Profile = Writer.writeBuffer();
readProfile(std::move(Profile));
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("foo", 0x1234);
+ auto R = Reader->getInstrProfRecord("foo", 0x1234);
EXPECT_THAT_ERROR(R.takeError(), Succeeded());
ASSERT_EQ(2U, R->Counts.size());
ASSERT_EQ(1U, R->Counts[0]);
@@ -251,7 +251,7 @@ TEST_F(InstrProfTest, test_writer_merge) {
auto Profile = Writer.writeBuffer();
readProfile(std::move(Profile));
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("func1", 0x1234);
+ auto R = Reader->getInstrProfRecord("func1", 0x1234);
EXPECT_THAT_ERROR(R.takeError(), Succeeded());
ASSERT_EQ(1U, R->Counts.size());
ASSERT_EQ(42U, R->Counts[0]);
@@ -600,7 +600,7 @@ TEST_F(InstrProfTest, test_memprof_merge) {
auto Profile = Writer.writeBuffer();
readProfile(std::move(Profile));
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("func1", 0x1234);
+ auto R = Reader->getInstrProfRecord("func1", 0x1234);
EXPECT_THAT_ERROR(R.takeError(), Succeeded());
ASSERT_EQ(1U, R->Counts.size());
ASSERT_EQ(42U, R->Counts[0]);
@@ -800,7 +800,7 @@ TEST_P(InstrProfReaderWriterTest, icall_and_vtable_data_read_write) {
// Set reader value prof data endianness.
Reader->setValueProfDataEndianness(getEndianness());
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
+ auto R = Reader->getInstrProfRecord("caller", 0x1234);
ASSERT_THAT_ERROR(R.takeError(), Succeeded());
// Test the number of instrumented indirect call sites and the number of
@@ -874,7 +874,7 @@ TEST_P(MaybeSparseInstrProfTest, annotate_vp_data) {
Writer.addRecord(std::move(Record), Err);
auto Profile = Writer.writeBuffer();
readProfile(std::move(Profile));
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
+ auto R = Reader->getInstrProfRecord("caller", 0x1234);
EXPECT_THAT_ERROR(R.takeError(), Succeeded());
LLVMContext Ctx;
@@ -1051,7 +1051,7 @@ TEST_P(MaybeSparseInstrProfTest, icall_and_vtable_data_merge) {
// Test the number of instrumented value sites and the number of profiled
// values for each site.
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
+ auto R = Reader->getInstrProfRecord("caller", 0x1234);
EXPECT_THAT_ERROR(R.takeError(), Succeeded());
// For indirect calls.
ASSERT_EQ(5U, R->getNumValueSites(IPVK_IndirectCallTarget));
@@ -1190,13 +1190,11 @@ TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_saturation) {
readProfile(std::move(Profile));
// Verify saturation of counts.
- Expected<InstrProfRecord> ReadRecord1 =
- Reader->getInstrProfRecord("foo", 0x1234);
+ auto ReadRecord1 = Reader->getInstrProfRecord("foo", 0x1234);
ASSERT_THAT_ERROR(ReadRecord1.takeError(), Succeeded());
EXPECT_EQ(MaxEdgeCount, ReadRecord1->Counts[0]);
- Expected<InstrProfRecord> ReadRecord2 =
- Reader->getInstrProfRecord("baz", 0x5678);
+ auto ReadRecord2 = Reader->getInstrProfRecord("baz", 0x5678);
ASSERT_TRUE(bool(ReadRecord2));
ASSERT_EQ(1U, ReadRecord2->getNumValueSites(ValueKind));
auto VD = ReadRecord2->getValueArrayForSite(ValueKind, 0);
@@ -1241,7 +1239,7 @@ TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_site_trunc) {
auto Profile = Writer.writeBuffer();
readProfile(std::move(Profile));
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
+ auto R = Reader->getInstrProfRecord("caller", 0x1234);
ASSERT_THAT_ERROR(R.takeError(), Succeeded());
ASSERT_EQ(2U, R->getNumValueSites(ValueKind));
auto VD = R->getValueArrayForSite(ValueKind, 0);
>From 9f3d3f6112f4f02a83184f66a16106c80f0fe328 Mon Sep 17 00:00:00 2001
From: Ellis Hoag <ellishoag at meta.com>
Date: Mon, 23 Jun 2025 17:35:25 -0700
Subject: [PATCH 2/3] move comments
---
.../lib/Transforms/Instrumentation/PGOInstrumentation.cpp | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index eddbf73bb6c1c..67a2e5cb4cc48 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1176,7 +1176,9 @@ class PGOUseFunc {
void handleInstrProfError(Error Err, uint64_t MismatchedFuncSum);
- // Get the profile record and handle errors if necessary.
+ /// Get the profile record, assign it to \p ProfileRecord, handle errors if
+ /// necessary, and assign \p ProgramMaxCount. \returns true if there are no
+ /// errors.
bool getRecord(IndexedInstrProfReader *PGOReader);
// Read counts for the instrumented BB from profile.
@@ -1459,8 +1461,8 @@ bool PGOUseFunc::getRecord(IndexedInstrProfReader *PGOReader) {
}
// Read the profile from ProfileFileName and assign the value to the
-// instrumented BB and the edges. This function also updates ProgramMaxCount.
-// Return true if the profile are successfully read, and false on errors.
+// instrumented BB and the edges. Return true if the profile are successfully
+// read, and false on errors.
bool PGOUseFunc::readCounters(bool &AllZeros,
InstrProfRecord::CountPseudoKind &PseudoKind) {
auto &Ctx = M->getContext();
>From 001164e698d4215e78ffef0f504b266902bfc7b9 Mon Sep 17 00:00:00 2001
From: Ellis Hoag <ellishoag at meta.com>
Date: Tue, 24 Jun 2025 08:18:08 -0700
Subject: [PATCH 3/3] change type of ProfileRecord
---
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index 67a2e5cb4cc48..6f06a260e238c 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1213,7 +1213,7 @@ class PGOUseFunc {
uint64_t getFuncHash() const { return FuncInfo.FunctionHash; }
// Return the profile record for this function;
- InstrProfRecord &getProfileRecord() { return ProfileRecord; }
+ NamedInstrProfRecord &getProfileRecord() { return ProfileRecord; }
// Return the auxiliary BB information.
PGOUseBBInfo &getBBInfo(const BasicBlock *BB) const {
@@ -1251,7 +1251,7 @@ class PGOUseFunc {
uint32_t ProfileCountSize = 0;
// ProfileRecord for this function.
- InstrProfRecord ProfileRecord;
+ NamedInstrProfRecord ProfileRecord;
// Function hotness info derived from profile.
FuncFreqAttr FreqAttr;
More information about the cfe-commits
mailing list