[llvm] r257143 - [PGO] Fix a bug in InstProfWriter addRecord
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 11 10:20:11 PST 2016
+1.
thanks,
David
On Mon, Jan 11, 2016 at 10:11 AM, Vedant Kumar <vsk at apple.com> wrote:
> Hi David,
>
> In the future, please use separate commits for bug fixes, new features, and refactoring.
>
> Squashing these into the same commit makes bisection more difficult.
>
> thanks
> vedant
>
>> On Jan 7, 2016, at 7:49 PM, Xinliang David Li via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: davidxl
>> Date: Thu Jan 7 21:49:59 2016
>> New Revision: 257143
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=257143&view=rev
>> Log:
>> [PGO] Fix a bug in InstProfWriter addRecord
>>
>> For a new record with weight != 1, only edge profiling
>> counters are scaled, VP data is not properly scaled.
>>
>> This patch refactors the code and fixes the problem.
>> Also added sort by count interface (for follow up patch).
>>
>>
>> Modified:
>> llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> llvm/trunk/lib/ProfileData/InstrProf.cpp
>> llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
>> llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=257143&r1=257142&r2=257143&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
>> +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Thu Jan 7 21:49:59 2016
>> @@ -355,11 +355,14 @@ struct InstrProfValueSiteRecord {
>> return left.Value < right.Value;
>> });
>> }
>> + /// Sort ValueData Descending by Count
>> + inline void sortByCount();
>>
>> /// Merge data from another InstrProfValueSiteRecord
>> /// Optionally scale merged counts by \p Weight.
>> - instrprof_error mergeValueData(InstrProfValueSiteRecord &Input,
>> - uint64_t Weight = 1);
>> + instrprof_error merge(InstrProfValueSiteRecord &Input, uint64_t Weight = 1);
>> + /// Scale up value profile data counts.
>> + instrprof_error scale(uint64_t Weight);
>> };
>>
>> /// Profiling information for a single function.
>> @@ -402,6 +405,19 @@ struct InstrProfRecord {
>> /// Optionally scale merged counts by \p Weight.
>> instrprof_error merge(InstrProfRecord &Other, uint64_t Weight = 1);
>>
>> + /// Scale up profile counts (including value profile data) by
>> + /// \p Weight.
>> + instrprof_error scale(uint64_t Weight);
>> +
>> + /// Sort value profile data (per site) by count.
>> + void sortValueData() {
>> + for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind) {
>> + std::vector<InstrProfValueSiteRecord> &SiteRecords =
>> + getValueSitesForKind(Kind);
>> + for (auto &SR : SiteRecords)
>> + SR.sortByCount();
>> + }
>> + }
>> /// Clear value data entries
>> void clearValueData() {
>> for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
>> @@ -436,6 +452,8 @@ private:
>> // Scale merged value counts by \p Weight.
>> instrprof_error mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,
>> uint64_t Weight);
>> + // Scale up value profile data count.
>> + instrprof_error scaleValueProfData(uint32_t ValueKind, uint64_t Weight);
>> };
>>
>> uint32_t InstrProfRecord::getNumValueKinds() const {
>> @@ -503,11 +521,22 @@ inline support::endianness getHostEndian
>> #define INSTR_PROF_VALUE_PROF_DATA
>> #include "llvm/ProfileData/InstrProfData.inc"
>>
>> - /*
>> - * Initialize the record for runtime value profile data.
>> - * Return 0 if the initialization is successful, otherwise
>> - * return 1.
>> - */
>> +void InstrProfValueSiteRecord::sortByCount() {
>> + ValueData.sort(
>> + [](const InstrProfValueData &left, const InstrProfValueData &right) {
>> + return left.Count > right.Count;
>> + });
>> + // Now truncate
>> + size_t max_s = INSTR_PROF_MAX_NUM_VAL_PER_SITE;
>> + if (ValueData.size() > max_s)
>> + ValueData.resize(max_s);
>> +}
>> +
>> +/*
>> +* Initialize the record for runtime value profile data.
>> +* Return 0 if the initialization is successful, otherwise
>> +* return 1.
>> +*/
>> int initializeValueProfRuntimeRecord(ValueProfRuntimeRecord *RuntimeRecord,
>> const uint16_t *NumValueSites,
>> ValueProfNode **Nodes);
>>
>> Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=257143&r1=257142&r2=257143&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
>> +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Thu Jan 7 21:49:59 2016
>> @@ -257,9 +257,8 @@ int readPGOFuncNameStrings(StringRef Nam
>> return 0;
>> }
>>
>> -instrprof_error
>> -InstrProfValueSiteRecord::mergeValueData(InstrProfValueSiteRecord &Input,
>> - uint64_t Weight) {
>> +instrprof_error InstrProfValueSiteRecord::merge(InstrProfValueSiteRecord &Input,
>> + uint64_t Weight) {
>> this->sortByTargetValues();
>> Input.sortByTargetValues();
>> auto I = ValueData.begin();
>> @@ -288,6 +287,17 @@ InstrProfValueSiteRecord::mergeValueData
>> return Result;
>> }
>>
>> +instrprof_error InstrProfValueSiteRecord::scale(uint64_t Weight) {
>> + instrprof_error Result = instrprof_error::success;
>> + for (auto I = ValueData.begin(), IE = ValueData.end(); I != IE; ++I) {
>> + bool Overflowed;
>> + I->Count = SaturatingMultiply(I->Count, Weight, &Overflowed);
>> + if (Overflowed)
>> + Result = instrprof_error::counter_overflow;
>> + }
>> + return Result;
>> +}
>> +
>> // Merge Value Profile data from Src record to this record for ValueKind.
>> // Scale merged value counts by \p Weight.
>> instrprof_error InstrProfRecord::mergeValueProfData(uint32_t ValueKind,
>> @@ -303,8 +313,7 @@ instrprof_error InstrProfRecord::mergeVa
>> Src.getValueSitesForKind(ValueKind);
>> instrprof_error Result = instrprof_error::success;
>> for (uint32_t I = 0; I < ThisNumValueSites; I++)
>> - MergeResult(Result,
>> - ThisSiteRecords[I].mergeValueData(OtherSiteRecords[I], Weight));
>> + MergeResult(Result, ThisSiteRecords[I].merge(OtherSiteRecords[I], Weight));
>> return Result;
>> }
>>
>> @@ -335,6 +344,32 @@ instrprof_error InstrProfRecord::merge(I
>>
>> return Result;
>> }
>> +
>> +instrprof_error InstrProfRecord::scaleValueProfData(uint32_t ValueKind,
>> + uint64_t Weight) {
>> + uint32_t ThisNumValueSites = getNumValueSites(ValueKind);
>> + std::vector<InstrProfValueSiteRecord> &ThisSiteRecords =
>> + getValueSitesForKind(ValueKind);
>> + instrprof_error Result = instrprof_error::success;
>> + for (uint32_t I = 0; I < ThisNumValueSites; I++)
>> + MergeResult(Result, ThisSiteRecords[I].scale(Weight));
>> + return Result;
>> +}
>> +
>> +instrprof_error InstrProfRecord::scale(uint64_t Weight) {
>> + instrprof_error Result = instrprof_error::success;
>> + for (auto &Count : this->Counts) {
>> + bool Overflowed;
>> + Count = SaturatingMultiply(Count, Weight, &Overflowed);
>> + if (Overflowed && Result == instrprof_error::success) {
>> + Result = instrprof_error::counter_overflow;
>> + }
>> + }
>> + for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
>> + MergeResult(Result, scaleValueProfData(Kind, Weight));
>> +
>> + return Result;
>> +}
>>
>> // Map indirect call target name hash to name string.
>> uint64_t InstrProfRecord::remapValue(uint64_t Value, uint32_t ValueKind,
>>
>> Modified: llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProfWriter.cpp?rev=257143&r1=257142&r2=257143&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/ProfileData/InstrProfWriter.cpp (original)
>> +++ llvm/trunk/lib/ProfileData/InstrProfWriter.cpp Thu Jan 7 21:49:59 2016
>> @@ -104,22 +104,14 @@ std::error_code InstrProfWriter::addReco
>> ProfileDataMap.insert(std::make_pair(I.Hash, InstrProfRecord()));
>> InstrProfRecord &Dest = Where->second;
>>
>> - instrprof_error Result;
>> + instrprof_error Result = instrprof_error::success;
>> if (NewFunc) {
>> // We've never seen a function with this name and hash, add it.
>> Dest = std::move(I);
>> // Fix up the name to avoid dangling reference.
>> Dest.Name = FunctionData.find(Dest.Name)->getKey();
>> - Result = instrprof_error::success;
>> - if (Weight > 1) {
>> - for (auto &Count : Dest.Counts) {
>> - bool Overflowed;
>> - Count = SaturatingMultiply(Count, Weight, &Overflowed);
>> - if (Overflowed && Result == instrprof_error::success) {
>> - Result = instrprof_error::counter_overflow;
>> - }
>> - }
>> - }
>> + if (Weight > 1)
>> + Result = Dest.scale(Weight);
>> } else {
>> // We're updating a function we've seen before.
>> Result = Dest.merge(I, Weight);
>>
>> Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=257143&r1=257142&r2=257143&view=diff
>> ==============================================================================
>> --- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)
>> +++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Thu Jan 7 21:49:59 2016
>> @@ -164,6 +164,62 @@ TEST_F(InstrProfTest, get_icall_data_rea
>> [](const InstrProfValueData &VD1, const InstrProfValueData &VD2) {
>> return VD1.Count > VD2.Count;
>> });
>> +
>> + ASSERT_EQ(3U, VD[0].Count);
>> + ASSERT_EQ(2U, VD[1].Count);
>> + ASSERT_EQ(1U, VD[2].Count);
>> +
>> + ASSERT_EQ(StringRef((const char *)VD[0].Value, 7), StringRef("callee3"));
>> + ASSERT_EQ(StringRef((const char *)VD[1].Value, 7), StringRef("callee2"));
>> + ASSERT_EQ(StringRef((const char *)VD[2].Value, 7), StringRef("callee1"));
>> +}
>> +
>> +TEST_F(InstrProfTest, get_icall_data_read_write_with_weight) {
>> + InstrProfRecord Record1("caller", 0x1234, {1, 2});
>> + InstrProfRecord Record2("callee1", 0x1235, {3, 4});
>> + InstrProfRecord Record3("callee2", 0x1235, {3, 4});
>> + InstrProfRecord Record4("callee3", 0x1235, {3, 4});
>> +
>> + // 4 value sites.
>> + 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);
>> + // No value profile data at the second site.
>> + Record1.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
>> + InstrProfValueData VD2[] = {{(uint64_t) "callee1", 1},
>> + {(uint64_t) "callee2", 2}};
>> + Record1.addValueData(IPVK_IndirectCallTarget, 2, VD2, 2, nullptr);
>> + InstrProfValueData VD3[] = {{(uint64_t) "callee1", 1}};
>> + Record1.addValueData(IPVK_IndirectCallTarget, 3, VD3, 1, nullptr);
>> +
>> + Writer.addRecord(std::move(Record1), 10);
>> + Writer.addRecord(std::move(Record2));
>> + Writer.addRecord(std::move(Record3));
>> + Writer.addRecord(std::move(Record4));
>> + auto Profile = Writer.writeBuffer();
>> + readProfile(std::move(Profile));
>> +
>> + ErrorOr<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
>> + ASSERT_TRUE(NoError(R.getError()));
>> + ASSERT_EQ(4U, R.get().getNumValueSites(IPVK_IndirectCallTarget));
>> + ASSERT_EQ(3U, R.get().getNumValueDataForSite(IPVK_IndirectCallTarget, 0));
>> + ASSERT_EQ(0U, R.get().getNumValueDataForSite(IPVK_IndirectCallTarget, 1));
>> + ASSERT_EQ(2U, R.get().getNumValueDataForSite(IPVK_IndirectCallTarget, 2));
>> + ASSERT_EQ(1U, R.get().getNumValueDataForSite(IPVK_IndirectCallTarget, 3));
>> +
>> + std::unique_ptr<InstrProfValueData[]> VD =
>> + R.get().getValueForSite(IPVK_IndirectCallTarget, 0);
>> + // Now sort the target acording to frequency.
>> + std::sort(&VD[0], &VD[3],
>> + [](const InstrProfValueData &VD1, const InstrProfValueData &VD2) {
>> + return VD1.Count > VD2.Count;
>> + });
>> + ASSERT_EQ(30U, VD[0].Count);
>> + ASSERT_EQ(20U, VD[1].Count);
>> + ASSERT_EQ(10U, VD[2].Count);
>> +
>> ASSERT_EQ(StringRef((const char *)VD[0].Value, 7), StringRef("callee3"));
>> ASSERT_EQ(StringRef((const char *)VD[1].Value, 7), StringRef("callee2"));
>> ASSERT_EQ(StringRef((const char *)VD[2].Value, 7), StringRef("callee1"));
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list