[llvm] r269222 - [ProfileData] Use SoftInstrProfErrors to count soft errors, NFC
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 14:41:39 PDT 2017
On Thu, Jun 29, 2017 at 2:24 PM Xinliang David Li <davidxl at google.com>
wrote:
> 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.
>
Ah, the data I have at the moment points to the actual InstrProfRecords
themselves, and the map entries they are within, account for 93% of memory
usage (85% SmallDenseMap::allocateBuckets -> malloc/new/new[], 8%
StringMapEntry::Create -> MallocAllocator::Allocate).
The std::vector allocation for the Counts (the value profiling vectors are
below the 1% threshold) are 1.32%.
> 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).
>
Sure - I can check, I think it's more than that.
Yep, in my prototype, adding one word to the sub-struct increases peak
memory usage from 5.1 -> 5.9 GB.
>
> 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/7ae7a838/attachment.html>
More information about the llvm-commits
mailing list