[llvm] r257143 - [PGO] Fix a bug in InstProfWriter addRecord
Vedant Kumar via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 11 10:11:38 PST 2016
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