[llvm] r269222 - [ProfileData] Use SoftInstrProfErrors to count soft errors, NFC
via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 10:07:02 PDT 2017
> On Jun 29, 2017, at 10:06 AM, Xinliang David Li <davidxl at google.com> wrote:
>
>
>
> On Thu, Jun 29, 2017 at 10:03 AM, <vsk at apple.com <mailto:vsk at apple.com>> wrote:
> I'm in favor of removing the counters from this struct, but keeping it as a member of InstrProfRecord [*]
>
>
> Do you mean the error counters? I am fine with removing them.
Yes, getNum*Mismatches have all gone unused.
vedant
>
> David
>
>
> I expected to use the counters in this struct to aid debugging, and to eventually implement better error reporting. This hasn't panned out: the counters haven't been much help (at least, not to me, I'm curious as to whether @davidxl has made use of them). It's totally fair to remove them until we have a better plan.
>
> Having a soft error tracker within InstrProfRecord makes error tracking very 'nice'. We can make sure an IPR isn't destroyed without its error state being considered. We can also handle copying/moving IPRs easily. @dblaikie The cost of a single instrprof_error field doesn't seem too high (at least, it'd be a lot better than what we have now). Would that work for you?
>
> thanks,
> vedant
>
> [*] I'm also volunteering to make the change, if there's agreement it's the right way to go :).
>
>> On Jun 29, 2017, at 9:44 AM, Xinliang David Li <davidxl at google.com <mailto:davidxl at google.com>> wrote:
>>
>> The size impact of this struct is indeed pretty large. We can consider compress the size of it significantly. For instance, making each member uint8_t. If the number of errors of each category > 255, it can be capped at 255 and the error message handling can be adjusted properly.
>>
>> David
>>
>> On Wed, Jun 28, 2017 at 11:57 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> This seems to have gone unused for a year so far - shall we remove it until there's an actual use case?
>>
>> I came across this because the sizeof(InstrProfRecord) contributes significantly to the memory footprint of llvm-profdata merge (I've made one big improvement so far (14GB -> 10GB for an example large profile)). I have a change in mind/prototyped that helps that (10GB -> 5GB) by moving the counters into a sub-struct of InstrProfRecord and using only that sub-struct in the InstrProfWriter (since it has the name and hash in the maps its using for lookup during merging - so they don't need to be duplicated in the values too). But this SoftInstrProfErrors is used from the counter-related functions and is quite large, so naively it would have to move into this sub-struct & take up lots of space.
>>
>> So at the very least I'd like to revisit the choice to make this a member, and instead go with the earlier version of this patch that wired it through function parameters instead - but given the lack of use, I think maybe it'd be better to remove this unused abstraction & go back to the simpler error handling that was present before.
>>
>> - Dave
>>
>> On Wed, May 11, 2016 at 12:48 PM Vedant Kumar via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> Author: vedantk
>> Date: Wed May 11 14:42:19 2016
>> New Revision: 269222
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=269222&view=rev <http://llvm.org/viewvc/llvm-project?rev=269222&view=rev>
>> Log:
>> [ProfileData] Use SoftInstrProfErrors to count soft errors, NFC
>>
>> Differential Revision: http://reviews.llvm.org/D20082 <http://reviews.llvm.org/D20082>
>>
>> Modified:
>> llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> llvm/trunk/lib/ProfileData/InstrProf.cpp
>> llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
>>
>> Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=269222&r1=269221&r2=269222&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=269222&r1=269221&r2=269222&view=diff>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
>> +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed May 11 14:42:19 2016
>> @@ -284,15 +284,51 @@ inline std::error_code make_error_code(i
>> return std::error_code(static_cast<int>(E), instrprof_category());
>> }
>>
>> -inline instrprof_error MergeResult(instrprof_error &Accumulator,
>> - instrprof_error Result) {
>> - // Prefer first error encountered as later errors may be secondary effects of
>> - // the initial problem.
>> - if (Accumulator == instrprof_error::success &&
>> - Result != instrprof_error::success)
>> - Accumulator = Result;
>> - return Accumulator;
>> -}
>> +class SoftInstrProfErrors {
>> + /// Count the number of soft instrprof_errors encountered and keep track of
>> + /// the first such error for reporting purposes.
>> +
>> + /// The first soft error encountered.
>> + instrprof_error FirstError;
>> +
>> + /// The number of hash mismatches.
>> + unsigned NumHashMismatches;
>> +
>> + /// The number of count mismatches.
>> + unsigned NumCountMismatches;
>> +
>> + /// The number of counter overflows.
>> + unsigned NumCounterOverflows;
>> +
>> + /// The number of value site count mismatches.
>> + unsigned NumValueSiteCountMismatches;
>> +
>> +public:
>> + SoftInstrProfErrors()
>> + : FirstError(instrprof_error::success), NumHashMismatches(0),
>> + NumCountMismatches(0), NumCounterOverflows(0),
>> + NumValueSiteCountMismatches(0) {}
>> +
>> + /// Track a soft error (\p IE) and increment its associated counter.
>> + void addError(instrprof_error IE);
>> +
>> + /// Get the number of hash mismatches.
>> + unsigned getNumHashMismatches() const { return NumHashMismatches; }
>> +
>> + /// Get the number of count mismatches.
>> + unsigned getNumCountMismatches() const { return NumCountMismatches; }
>> +
>> + /// Get the number of counter overflows.
>> + unsigned getNumCounterOverflows() const { return NumCounterOverflows; }
>> +
>> + /// Get the number of value site count mismatches.
>> + unsigned getNumValueSiteCountMismatches() const {
>> + return NumValueSiteCountMismatches;
>> + }
>> +
>> + /// Return an error code for the first encountered error.
>> + std::error_code getError() const { return make_error_code(FirstError); }
>> +};
>>
>> namespace object {
>> class SectionRef;
>> @@ -465,19 +501,21 @@ struct InstrProfValueSiteRecord {
>>
>> /// Merge data from another InstrProfValueSiteRecord
>> /// Optionally scale merged counts by \p Weight.
>> - instrprof_error merge(InstrProfValueSiteRecord &Input, uint64_t Weight = 1);
>> + void merge(SoftInstrProfErrors &SIPE, InstrProfValueSiteRecord &Input,
>> + uint64_t Weight = 1);
>> /// Scale up value profile data counts.
>> - instrprof_error scale(uint64_t Weight);
>> + void scale(SoftInstrProfErrors &SIPE, uint64_t Weight);
>> };
>>
>> /// Profiling information for a single function.
>> struct InstrProfRecord {
>> - InstrProfRecord() {}
>> + InstrProfRecord() : SIPE() {}
>> InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t> Counts)
>> - : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}
>> + : Name(Name), Hash(Hash), Counts(std::move(Counts)), SIPE() {}
>> StringRef Name;
>> uint64_t Hash;
>> std::vector<uint64_t> Counts;
>> + SoftInstrProfErrors SIPE;
>>
>> typedef std::vector<std::pair<uint64_t, uint64_t>> ValueMapType;
>>
>> @@ -512,11 +550,11 @@ struct InstrProfRecord {
>>
>> /// Merge the counts in \p Other into this one.
>> /// Optionally scale merged counts by \p Weight.
>> - instrprof_error merge(InstrProfRecord &Other, uint64_t Weight = 1);
>> + void merge(InstrProfRecord &Other, uint64_t Weight = 1);
>>
>> /// Scale up profile counts (including value profile data) by
>> /// \p Weight.
>> - instrprof_error scale(uint64_t Weight);
>> + void scale(uint64_t Weight);
>>
>> /// Sort value profile data (per site) by count.
>> void sortValueData() {
>> @@ -533,6 +571,9 @@ struct InstrProfRecord {
>> getValueSitesForKind(Kind).clear();
>> }
>>
>> + /// Get the error contained within the record's soft error counter.
>> + std::error_code getError() const { return SIPE.getError(); }
>> +
>> private:
>> std::vector<InstrProfValueSiteRecord> IndirectCallSites;
>> const std::vector<InstrProfValueSiteRecord> &
>> @@ -559,10 +600,10 @@ private:
>>
>> // Merge Value Profile data from Src record to this record for ValueKind.
>> // Scale merged value counts by \p Weight.
>> - instrprof_error mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,
>> - uint64_t Weight);
>> + void mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,
>> + uint64_t Weight);
>> // Scale up value profile data count.
>> - instrprof_error scaleValueProfData(uint32_t ValueKind, uint64_t Weight);
>> + void scaleValueProfData(uint32_t ValueKind, uint64_t Weight);
>> };
>>
>> uint32_t InstrProfRecord::getNumValueKinds() const {
>>
>> Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=269222&r1=269221&r2=269222&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=269222&r1=269221&r2=269222&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
>> +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Wed May 11 14:42:19 2016
>> @@ -80,6 +80,31 @@ const std::error_category &llvm::instrpr
>>
>> namespace llvm {
>>
>> +void SoftInstrProfErrors::addError(instrprof_error IE) {
>> + if (IE == instrprof_error::success)
>> + return;
>> +
>> + if (FirstError == instrprof_error::success)
>> + FirstError = IE;
>> +
>> + switch (IE) {
>> + case instrprof_error::hash_mismatch:
>> + ++NumHashMismatches;
>> + break;
>> + case instrprof_error::count_mismatch:
>> + ++NumCountMismatches;
>> + break;
>> + case instrprof_error::counter_overflow:
>> + ++NumCounterOverflows;
>> + break;
>> + case instrprof_error::value_site_count_mismatch:
>> + ++NumValueSiteCountMismatches;
>> + break;
>> + default:
>> + llvm_unreachable("Not a soft error");
>> + }
>> +}
>> +
>> std::string getPGOFuncName(StringRef RawFuncName,
>> GlobalValue::LinkageTypes Linkage,
>> StringRef FileName,
>> @@ -291,13 +316,13 @@ std::error_code readPGOFuncNameStrings(S
>> return make_error_code(instrprof_error::success);
>> }
>>
>> -instrprof_error InstrProfValueSiteRecord::merge(InstrProfValueSiteRecord &Input,
>> - uint64_t Weight) {
>> +void InstrProfValueSiteRecord::merge(SoftInstrProfErrors &SIPE,
>> + InstrProfValueSiteRecord &Input,
>> + uint64_t Weight) {
>> this->sortByTargetValues();
>> Input.sortByTargetValues();
>> auto I = ValueData.begin();
>> auto IE = ValueData.end();
>> - instrprof_error Result = instrprof_error::success;
>> for (auto J = Input.ValueData.begin(), JE = Input.ValueData.end(); J != JE;
>> ++J) {
>> while (I != IE && I->Value < J->Value)
>> @@ -306,92 +331,80 @@ instrprof_error InstrProfValueSiteRecord
>> bool Overflowed;
>> I->Count = SaturatingMultiplyAdd(J->Count, Weight, I->Count, &Overflowed);
>> if (Overflowed)
>> - Result = instrprof_error::counter_overflow;
>> + SIPE.addError(instrprof_error::counter_overflow);
>> ++I;
>> continue;
>> }
>> ValueData.insert(I, *J);
>> }
>> - return Result;
>> }
>>
>> -instrprof_error InstrProfValueSiteRecord::scale(uint64_t Weight) {
>> - instrprof_error Result = instrprof_error::success;
>> +void InstrProfValueSiteRecord::scale(SoftInstrProfErrors &SIPE,
>> + uint64_t Weight) {
>> 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;
>> + SIPE.addError(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,
>> - InstrProfRecord &Src,
>> - uint64_t Weight) {
>> +void InstrProfRecord::mergeValueProfData(uint32_t ValueKind,
>> + InstrProfRecord &Src,
>> + uint64_t Weight) {
>> uint32_t ThisNumValueSites = getNumValueSites(ValueKind);
>> uint32_t OtherNumValueSites = Src.getNumValueSites(ValueKind);
>> - if (ThisNumValueSites != OtherNumValueSites)
>> - return instrprof_error::value_site_count_mismatch;
>> + if (ThisNumValueSites != OtherNumValueSites) {
>> + SIPE.addError(instrprof_error::value_site_count_mismatch);
>> + return;
>> + }
>> std::vector<InstrProfValueSiteRecord> &ThisSiteRecords =
>> getValueSitesForKind(ValueKind);
>> std::vector<InstrProfValueSiteRecord> &OtherSiteRecords =
>> Src.getValueSitesForKind(ValueKind);
>> - instrprof_error Result = instrprof_error::success;
>> for (uint32_t I = 0; I < ThisNumValueSites; I++)
>> - MergeResult(Result, ThisSiteRecords[I].merge(OtherSiteRecords[I], Weight));
>> - return Result;
>> + ThisSiteRecords[I].merge(SIPE, OtherSiteRecords[I], Weight);
>> }
>>
>> -instrprof_error InstrProfRecord::merge(InstrProfRecord &Other,
>> - uint64_t Weight) {
>> +void InstrProfRecord::merge(InstrProfRecord &Other, uint64_t Weight) {
>> // If the number of counters doesn't match we either have bad data
>> // or a hash collision.
>> - if (Counts.size() != Other.Counts.size())
>> - return instrprof_error::count_mismatch;
>> -
>> - instrprof_error Result = instrprof_error::success;
>> + if (Counts.size() != Other.Counts.size()) {
>> + SIPE.addError(instrprof_error::count_mismatch);
>> + return;
>> + }
>>
>> for (size_t I = 0, E = Other.Counts.size(); I < E; ++I) {
>> bool Overflowed;
>> Counts[I] =
>> SaturatingMultiplyAdd(Other.Counts[I], Weight, Counts[I], &Overflowed);
>> if (Overflowed)
>> - Result = instrprof_error::counter_overflow;
>> + SIPE.addError(instrprof_error::counter_overflow);
>> }
>>
>> for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
>> - MergeResult(Result, mergeValueProfData(Kind, Other, Weight));
>> -
>> - return Result;
>> + mergeValueProfData(Kind, Other, Weight);
>> }
>>
>> -instrprof_error InstrProfRecord::scaleValueProfData(uint32_t ValueKind,
>> - uint64_t Weight) {
>> +void 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;
>> + ThisSiteRecords[I].scale(SIPE, Weight);
>> }
>>
>> -instrprof_error InstrProfRecord::scale(uint64_t Weight) {
>> - instrprof_error Result = instrprof_error::success;
>> +void InstrProfRecord::scale(uint64_t Weight) {
>> for (auto &Count : this->Counts) {
>> bool Overflowed;
>> Count = SaturatingMultiply(Count, Weight, &Overflowed);
>> - if (Overflowed && Result == instrprof_error::success) {
>> - Result = instrprof_error::counter_overflow;
>> - }
>> + if (Overflowed)
>> + SIPE.addError(instrprof_error::counter_overflow);
>> }
>> for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
>> - MergeResult(Result, scaleValueProfData(Kind, Weight));
>> -
>> - return Result;
>> + scaleValueProfData(Kind, Weight);
>> }
>>
>> // Map indirect call target name hash to name string.
>>
>> Modified: llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProfWriter.cpp?rev=269222&r1=269221&r2=269222&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProfWriter.cpp?rev=269222&r1=269221&r2=269222&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/ProfileData/InstrProfWriter.cpp (original)
>> +++ llvm/trunk/lib/ProfileData/InstrProfWriter.cpp Wed May 11 14:42:19 2016
>> @@ -166,22 +166,21 @@ std::error_code InstrProfWriter::addReco
>> ProfileDataMap.insert(std::make_pair(I.Hash, InstrProfRecord()));
>> InstrProfRecord &Dest = Where->second;
>>
>> - 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();
>> if (Weight > 1)
>> - Result = Dest.scale(Weight);
>> + Dest.scale(Weight);
>> } else {
>> // We're updating a function we've seen before.
>> - Result = Dest.merge(I, Weight);
>> + Dest.merge(I, Weight);
>> }
>>
>> Dest.sortValueData();
>>
>> - return Result;
>> + return Dest.getError();
>> }
>>
>> bool InstrProfWriter::shouldEncodeData(const ProfilingData &PD) {
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170629/b6d63c5f/attachment.html>
More information about the llvm-commits
mailing list