[llvm] r269222 - [ProfileData] Use SoftInstrProfErrors to count soft errors, NFC
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 14:24:14 PDT 2017
On Thu, Jun 29, 2017 at 2:16 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Thu, Jun 29, 2017 at 10:03 AM <vsk at apple.com> wrote:
>
>> I'm in favor of removing the counters from this struct, but keeping it as
>> a member of InstrProfRecord [*]
>>
>> 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?
>>
>
> I'd really be inclined to push for removing it entirely - keeping the
> error (even as a byte rather than a word) in the counters sub-struct would
> be a 25% increase in size for this sub-struct (it'd have 3 words for the
> Counts vector, one word for the unique_ptr to the value profiling stats,
> then adding even another byte would add a 5th word (due to padding/rounding
> up)).
>
25% increase is over-stated :) Remember that the counters vector also has
dynamic allocated array with size that are multiples of uint64. I think
we can keep it for now unless we have data to show that the error field
creates significant overhead (e.g > 5% overall memory increase).
David
>
> I agree that error handling is really important - but like the original
> implementation you posted, I think error handling at the point of failure
> is generally a better idea (rather than having to keep track of whether
> this InstrProfRecord has been queried for its failures or not, etc - you
> call the function that can fail, you handle the failure).
>
> - Dave
>
>
>>
>> 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>
>> 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>
>> 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> 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
>>>> Log:
>>>> [ProfileData] Use SoftInstrProfErrors to count soft errors, NFC
>>>>
>>>> Differential Revision: 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
>>>> ============================================================
>>>> ==================
>>>> --- 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
>>>> ============================================================
>>>> ==================
>>>> --- 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
>>>> ============================================================
>>>> ==================
>>>> --- 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
>>>> 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/8927b36b/attachment.html>
More information about the llvm-commits
mailing list