[llvm] r269222 - [ProfileData] Use SoftInstrProfErrors to count soft errors, NFC
via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 10:03:52 PDT 2017
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?
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 <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/e6ddf32f/attachment.html>
More information about the llvm-commits
mailing list