[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:16:27 PDT 2017
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)).
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/efee7184/attachment.html>
More information about the llvm-commits
mailing list