[llvm] r269222 - [ProfileData] Use SoftInstrProfErrors to count soft errors, NFC
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 16:58:48 PDT 2017
On Thu, Jun 29, 2017 at 4:57 PM <vsk at apple.com> wrote:
> On Jun 29, 2017, at 4:52 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Jun 29, 2017 at 4:48 PM <vsk at apple.com> wrote:
>
>> On Jun 29, 2017, at 4:38 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Thu, Jun 29, 2017 at 4:27 PM <vsk at apple.com> wrote:
>>
>>> On Jun 29, 2017, at 4:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>
>>>
>>> On Thu, Jun 29, 2017 at 4:09 PM <vsk at apple.com> wrote:
>>>
>>>> On Jun 29, 2017, at 2:41 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> 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.
>>>>
>>>>
>>>> This is enough to convince me we should get rid of the error member.
>>>> It's more typing to thread the error state through callers, but it looks
>>>> like it's worth it.
>>>>
>>>
>>> Cool cool - I'm not sure how nice it'd be, but since we've moved to
>>> llvm::Error since the initial work - would it be possible/nice to just
>>> return Error from these functions, then it wouldn't be so many extra
>>> function parameters, etc?
>>>
>>>
>>> It's possible, but a bit cumbersome. E.g here's what error tracking
>>> looks like in a loop now:
>>>
>>> void merge(SoftInstrProfErrors &SIPE) {
>>> for each counter {
>>> add them
>>> if (overflow)
>>> SIPE.addError(counter_overflow);
>>> }
>>>
>>> Were we to return Error everywhere, you'd need something like this:
>>>
>>> Error merge() {
>>> Error E = Error::success();
>>> for each counter {
>>> add them
>>> if (overflow)
>>> if (!E)
>>> E = make_error<...>(counter_overflow);
>>> }
>>> return E;
>>> }
>>>
>>
>> Any chance of failing fast?
>>
>> Error merge() {
>> for each counter {
>> add them
>> if (overflow)
>> return make_error<...>(counter_overflow);
>> }
>> return Error::success();
>> }
>>
>>
>> I don't think so, we need a way to resume these operations when we
>> encounter a soft error.
>>
>
> That's sort of what I was wondering about/questioning. Are these more like
> warnings than errors? Or why is it important to resume/continue after these
> failures?
>
>
> They're more like warnings: it provides a heads-up to users when their
> profiles are getting stale (hash mismatches), or when imprecision is
> creeping in to the profiles (counter overflow). It's important that we be
> able to recover from these issues, because e.g clang-based profiling
> instrumentation is designed to work with profiles which are a bit stale.
> The original motivation was to make it possible for users to store profiles
> on some server, and then have project builds reuse that profile data,
> instead of obtaining fresh training data every time the project needs to be
> built.
>
Ah, great - thanks for the context!
Yeah, a callback sounds totally great for this!
- Dave
>
> vedant
>
>
> ("more what you'd call... guidelines" ;) )
>
>
>>
>>
>> If these Errors are recoverable (as might be indicated by the 'soft' in
>> SoftInstrProfError) maybe it'd be better to pass a callback through (while
>> you're adding this parameter everywhere anyway) - function_ref<void(Error)>
>>
>>
>> That way one caller could pass a callback that does archive all the
>> Errors like what SoftInstrProfError was doing (without baking it in) &
>> existing callers can print the first Error and then use a boolean flag to
>> ignore the rest?
>>
>>
>> Having a callback sounds good to me, I don't know why I hadn't considered
>> that.
>>
>
> Dandy :)
>
>
>>
>> vedant
>>
>> Feel free to tell me such redesigns are a bit out of scope, etc - only
>> trying to offer some ideas, I'm mostly motivated to address the memory
>> usage right now & don't mean to make it into a bigger project than it needs
>> to be.
>>
>> - Dave
>>
>>
>>>
>>> Also, every time you call a function which returns an Error, you have to
>>> decide how to update the current function's error state. To preserve the
>>> current behavior, you'd only update the current error state if it is
>>> Error::success(), otherwise you'd have to consumeError() the new error and
>>> drop it.
>>>
>>> So I think it'll be more convenient to keep the soft error tracking
>>> class, but have it passed around by callers. It costs one extra parameter
>>> per method but it might end up making things cleaner.
>>>
>>> Of course if you do try returning Error everywhere and find that it's
>>> simple/ergonomic I'm open to that.
>>>
>>> vedant
>>>
>>>
>>> Anyway, whatever works best for you - just a thought.
>>>
>>>
>>>>
>>>> vedant
>>>>
>>>>
>>>>
>>>>>
>>>>> 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/22e506d4/attachment.html>
More information about the llvm-commits
mailing list