<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 29, 2017 at 10:03 AM <<a href="mailto:vsk@apple.com">vsk@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>I'm in favor of removing the counters from this struct, but keeping it as a member of InstrProfRecord [*]</div><div><br></div><div>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.</div><div><br></div><div>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?</div></div></blockquote><div><br></div><div>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)).<br><br>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).<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>thanks,</div><div>vedant</div><div><br></div><div>[*] I'm also volunteering to make the change, if there's agreement it's the right way to go :).</div></div><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Jun 29, 2017, at 9:44 AM, Xinliang David Li <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>> wrote:</div><br class="m_7015459915651500388Apple-interchange-newline"><div><div dir="ltr">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.<div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 28, 2017 at 11:57 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">This seems to have gone unused for a year so far - shall we remove it until there's an actual use case?<br><br>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.<br><br>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.<br><br>- Dave<br><br><div class="gmail_quote"><div dir="ltr">On Wed, May 11, 2016 at 12:48 PM Vedant Kumar via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: vedantk<br>
Date: Wed May 11 14:42:19 2016<br>
New Revision: 269222<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=269222&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=269222&view=rev</a><br>
Log:<br>
[ProfileData] Use SoftInstrProfErrors to count soft errors, NFC<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D20082" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20082</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ProfileData/InstrProf.h<br>
llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
llvm/trunk/lib/ProfileData/InstrProfWriter.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=269222&r1=269221&r2=269222&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=269222&r1=269221&r2=269222&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)<br>
+++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed May 11 14:42:19 2016<br>
@@ -284,15 +284,51 @@ inline std::error_code make_error_code(i<br>
return std::error_code(static_cast<int>(E), instrprof_category());<br>
}<br>
<br>
-inline instrprof_error MergeResult(instrprof_error &Accumulator,<br>
- instrprof_error Result) {<br>
- // Prefer first error encountered as later errors may be secondary effects of<br>
- // the initial problem.<br>
- if (Accumulator == instrprof_error::success &&<br>
- Result != instrprof_error::success)<br>
- Accumulator = Result;<br>
- return Accumulator;<br>
-}<br>
+class SoftInstrProfErrors {<br>
+ /// Count the number of soft instrprof_errors encountered and keep track of<br>
+ /// the first such error for reporting purposes.<br>
+<br>
+ /// The first soft error encountered.<br>
+ instrprof_error FirstError;<br>
+<br>
+ /// The number of hash mismatches.<br>
+ unsigned NumHashMismatches;<br>
+<br>
+ /// The number of count mismatches.<br>
+ unsigned NumCountMismatches;<br>
+<br>
+ /// The number of counter overflows.<br>
+ unsigned NumCounterOverflows;<br>
+<br>
+ /// The number of value site count mismatches.<br>
+ unsigned NumValueSiteCountMismatches;<br>
+<br>
+public:<br>
+ SoftInstrProfErrors()<br>
+ : FirstError(instrprof_error::success), NumHashMismatches(0),<br>
+ NumCountMismatches(0), NumCounterOverflows(0),<br>
+ NumValueSiteCountMismatches(0) {}<br>
+<br>
+ /// Track a soft error (\p IE) and increment its associated counter.<br>
+ void addError(instrprof_error IE);<br>
+<br>
+ /// Get the number of hash mismatches.<br>
+ unsigned getNumHashMismatches() const { return NumHashMismatches; }<br>
+<br>
+ /// Get the number of count mismatches.<br>
+ unsigned getNumCountMismatches() const { return NumCountMismatches; }<br>
+<br>
+ /// Get the number of counter overflows.<br>
+ unsigned getNumCounterOverflows() const { return NumCounterOverflows; }<br>
+<br>
+ /// Get the number of value site count mismatches.<br>
+ unsigned getNumValueSiteCountMismatches() const {<br>
+ return NumValueSiteCountMismatches;<br>
+ }<br>
+<br>
+ /// Return an error code for the first encountered error.<br>
+ std::error_code getError() const { return make_error_code(FirstError); }<br>
+};<br>
<br>
namespace object {<br>
class SectionRef;<br>
@@ -465,19 +501,21 @@ struct InstrProfValueSiteRecord {<br>
<br>
/// Merge data from another InstrProfValueSiteRecord<br>
/// Optionally scale merged counts by \p Weight.<br>
- instrprof_error merge(InstrProfValueSiteRecord &Input, uint64_t Weight = 1);<br>
+ void merge(SoftInstrProfErrors &SIPE, InstrProfValueSiteRecord &Input,<br>
+ uint64_t Weight = 1);<br>
/// Scale up value profile data counts.<br>
- instrprof_error scale(uint64_t Weight);<br>
+ void scale(SoftInstrProfErrors &SIPE, uint64_t Weight);<br>
};<br>
<br>
/// Profiling information for a single function.<br>
struct InstrProfRecord {<br>
- InstrProfRecord() {}<br>
+ InstrProfRecord() : SIPE() {}<br>
InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t> Counts)<br>
- : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}<br>
+ : Name(Name), Hash(Hash), Counts(std::move(Counts)), SIPE() {}<br>
StringRef Name;<br>
uint64_t Hash;<br>
std::vector<uint64_t> Counts;<br>
+ SoftInstrProfErrors SIPE;<br>
<br>
typedef std::vector<std::pair<uint64_t, uint64_t>> ValueMapType;<br>
<br>
@@ -512,11 +550,11 @@ struct InstrProfRecord {<br>
<br>
/// Merge the counts in \p Other into this one.<br>
/// Optionally scale merged counts by \p Weight.<br>
- instrprof_error merge(InstrProfRecord &Other, uint64_t Weight = 1);<br>
+ void merge(InstrProfRecord &Other, uint64_t Weight = 1);<br>
<br>
/// Scale up profile counts (including value profile data) by<br>
/// \p Weight.<br>
- instrprof_error scale(uint64_t Weight);<br>
+ void scale(uint64_t Weight);<br>
<br>
/// Sort value profile data (per site) by count.<br>
void sortValueData() {<br>
@@ -533,6 +571,9 @@ struct InstrProfRecord {<br>
getValueSitesForKind(Kind).clear();<br>
}<br>
<br>
+ /// Get the error contained within the record's soft error counter.<br>
+ std::error_code getError() const { return SIPE.getError(); }<br>
+<br>
private:<br>
std::vector<InstrProfValueSiteRecord> IndirectCallSites;<br>
const std::vector<InstrProfValueSiteRecord> &<br>
@@ -559,10 +600,10 @@ private:<br>
<br>
// Merge Value Profile data from Src record to this record for ValueKind.<br>
// Scale merged value counts by \p Weight.<br>
- instrprof_error mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,<br>
- uint64_t Weight);<br>
+ void mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,<br>
+ uint64_t Weight);<br>
// Scale up value profile data count.<br>
- instrprof_error scaleValueProfData(uint32_t ValueKind, uint64_t Weight);<br>
+ void scaleValueProfData(uint32_t ValueKind, uint64_t Weight);<br>
};<br>
<br>
uint32_t InstrProfRecord::getNumValueKinds() const {<br>
<br>
Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=269222&r1=269221&r2=269222&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=269222&r1=269221&r2=269222&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)<br>
+++ llvm/trunk/lib/ProfileData/InstrProf.cpp Wed May 11 14:42:19 2016<br>
@@ -80,6 +80,31 @@ const std::error_category &llvm::instrpr<br>
<br>
namespace llvm {<br>
<br>
+void SoftInstrProfErrors::addError(instrprof_error IE) {<br>
+ if (IE == instrprof_error::success)<br>
+ return;<br>
+<br>
+ if (FirstError == instrprof_error::success)<br>
+ FirstError = IE;<br>
+<br>
+ switch (IE) {<br>
+ case instrprof_error::hash_mismatch:<br>
+ ++NumHashMismatches;<br>
+ break;<br>
+ case instrprof_error::count_mismatch:<br>
+ ++NumCountMismatches;<br>
+ break;<br>
+ case instrprof_error::counter_overflow:<br>
+ ++NumCounterOverflows;<br>
+ break;<br>
+ case instrprof_error::value_site_count_mismatch:<br>
+ ++NumValueSiteCountMismatches;<br>
+ break;<br>
+ default:<br>
+ llvm_unreachable("Not a soft error");<br>
+ }<br>
+}<br>
+<br>
std::string getPGOFuncName(StringRef RawFuncName,<br>
GlobalValue::LinkageTypes Linkage,<br>
StringRef FileName,<br>
@@ -291,13 +316,13 @@ std::error_code readPGOFuncNameStrings(S<br>
return make_error_code(instrprof_error::success);<br>
}<br>
<br>
-instrprof_error InstrProfValueSiteRecord::merge(InstrProfValueSiteRecord &Input,<br>
- uint64_t Weight) {<br>
+void InstrProfValueSiteRecord::merge(SoftInstrProfErrors &SIPE,<br>
+ InstrProfValueSiteRecord &Input,<br>
+ uint64_t Weight) {<br>
this->sortByTargetValues();<br>
Input.sortByTargetValues();<br>
auto I = ValueData.begin();<br>
auto IE = ValueData.end();<br>
- instrprof_error Result = instrprof_error::success;<br>
for (auto J = Input.ValueData.begin(), JE = Input.ValueData.end(); J != JE;<br>
++J) {<br>
while (I != IE && I->Value < J->Value)<br>
@@ -306,92 +331,80 @@ instrprof_error InstrProfValueSiteRecord<br>
bool Overflowed;<br>
I->Count = SaturatingMultiplyAdd(J->Count, Weight, I->Count, &Overflowed);<br>
if (Overflowed)<br>
- Result = instrprof_error::counter_overflow;<br>
+ SIPE.addError(instrprof_error::counter_overflow);<br>
++I;<br>
continue;<br>
}<br>
ValueData.insert(I, *J);<br>
}<br>
- return Result;<br>
}<br>
<br>
-instrprof_error InstrProfValueSiteRecord::scale(uint64_t Weight) {<br>
- instrprof_error Result = instrprof_error::success;<br>
+void InstrProfValueSiteRecord::scale(SoftInstrProfErrors &SIPE,<br>
+ uint64_t Weight) {<br>
for (auto I = ValueData.begin(), IE = ValueData.end(); I != IE; ++I) {<br>
bool Overflowed;<br>
I->Count = SaturatingMultiply(I->Count, Weight, &Overflowed);<br>
if (Overflowed)<br>
- Result = instrprof_error::counter_overflow;<br>
+ SIPE.addError(instrprof_error::counter_overflow);<br>
}<br>
- return Result;<br>
}<br>
<br>
// Merge Value Profile data from Src record to this record for ValueKind.<br>
// Scale merged value counts by \p Weight.<br>
-instrprof_error InstrProfRecord::mergeValueProfData(uint32_t ValueKind,<br>
- InstrProfRecord &Src,<br>
- uint64_t Weight) {<br>
+void InstrProfRecord::mergeValueProfData(uint32_t ValueKind,<br>
+ InstrProfRecord &Src,<br>
+ uint64_t Weight) {<br>
uint32_t ThisNumValueSites = getNumValueSites(ValueKind);<br>
uint32_t OtherNumValueSites = Src.getNumValueSites(ValueKind);<br>
- if (ThisNumValueSites != OtherNumValueSites)<br>
- return instrprof_error::value_site_count_mismatch;<br>
+ if (ThisNumValueSites != OtherNumValueSites) {<br>
+ SIPE.addError(instrprof_error::value_site_count_mismatch);<br>
+ return;<br>
+ }<br>
std::vector<InstrProfValueSiteRecord> &ThisSiteRecords =<br>
getValueSitesForKind(ValueKind);<br>
std::vector<InstrProfValueSiteRecord> &OtherSiteRecords =<br>
Src.getValueSitesForKind(ValueKind);<br>
- instrprof_error Result = instrprof_error::success;<br>
for (uint32_t I = 0; I < ThisNumValueSites; I++)<br>
- MergeResult(Result, ThisSiteRecords[I].merge(OtherSiteRecords[I], Weight));<br>
- return Result;<br>
+ ThisSiteRecords[I].merge(SIPE, OtherSiteRecords[I], Weight);<br>
}<br>
<br>
-instrprof_error InstrProfRecord::merge(InstrProfRecord &Other,<br>
- uint64_t Weight) {<br>
+void InstrProfRecord::merge(InstrProfRecord &Other, uint64_t Weight) {<br>
// If the number of counters doesn't match we either have bad data<br>
// or a hash collision.<br>
- if (Counts.size() != Other.Counts.size())<br>
- return instrprof_error::count_mismatch;<br>
-<br>
- instrprof_error Result = instrprof_error::success;<br>
+ if (Counts.size() != Other.Counts.size()) {<br>
+ SIPE.addError(instrprof_error::count_mismatch);<br>
+ return;<br>
+ }<br>
<br>
for (size_t I = 0, E = Other.Counts.size(); I < E; ++I) {<br>
bool Overflowed;<br>
Counts[I] =<br>
SaturatingMultiplyAdd(Other.Counts[I], Weight, Counts[I], &Overflowed);<br>
if (Overflowed)<br>
- Result = instrprof_error::counter_overflow;<br>
+ SIPE.addError(instrprof_error::counter_overflow);<br>
}<br>
<br>
for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)<br>
- MergeResult(Result, mergeValueProfData(Kind, Other, Weight));<br>
-<br>
- return Result;<br>
+ mergeValueProfData(Kind, Other, Weight);<br>
}<br>
<br>
-instrprof_error InstrProfRecord::scaleValueProfData(uint32_t ValueKind,<br>
- uint64_t Weight) {<br>
+void InstrProfRecord::scaleValueProfData(uint32_t ValueKind, uint64_t Weight) {<br>
uint32_t ThisNumValueSites = getNumValueSites(ValueKind);<br>
std::vector<InstrProfValueSiteRecord> &ThisSiteRecords =<br>
getValueSitesForKind(ValueKind);<br>
- instrprof_error Result = instrprof_error::success;<br>
for (uint32_t I = 0; I < ThisNumValueSites; I++)<br>
- MergeResult(Result, ThisSiteRecords[I].scale(Weight));<br>
- return Result;<br>
+ ThisSiteRecords[I].scale(SIPE, Weight);<br>
}<br>
<br>
-instrprof_error InstrProfRecord::scale(uint64_t Weight) {<br>
- instrprof_error Result = instrprof_error::success;<br>
+void InstrProfRecord::scale(uint64_t Weight) {<br>
for (auto &Count : this->Counts) {<br>
bool Overflowed;<br>
Count = SaturatingMultiply(Count, Weight, &Overflowed);<br>
- if (Overflowed && Result == instrprof_error::success) {<br>
- Result = instrprof_error::counter_overflow;<br>
- }<br>
+ if (Overflowed)<br>
+ SIPE.addError(instrprof_error::counter_overflow);<br>
}<br>
for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)<br>
- MergeResult(Result, scaleValueProfData(Kind, Weight));<br>
-<br>
- return Result;<br>
+ scaleValueProfData(Kind, Weight);<br>
}<br>
<br>
// Map indirect call target name hash to name string.<br>
<br>
Modified: llvm/trunk/lib/ProfileData/InstrProfWriter.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProfWriter.cpp?rev=269222&r1=269221&r2=269222&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProfWriter.cpp?rev=269222&r1=269221&r2=269222&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/ProfileData/InstrProfWriter.cpp (original)<br>
+++ llvm/trunk/lib/ProfileData/InstrProfWriter.cpp Wed May 11 14:42:19 2016<br>
@@ -166,22 +166,21 @@ std::error_code InstrProfWriter::addReco<br>
ProfileDataMap.insert(std::make_pair(I.Hash, InstrProfRecord()));<br>
InstrProfRecord &Dest = Where->second;<br>
<br>
- instrprof_error Result = instrprof_error::success;<br>
if (NewFunc) {<br>
// We've never seen a function with this name and hash, add it.<br>
Dest = std::move(I);<br>
// Fix up the name to avoid dangling reference.<br>
Dest.Name = FunctionData.find(Dest.Name)->getKey();<br>
if (Weight > 1)<br>
- Result = Dest.scale(Weight);<br>
+ Dest.scale(Weight);<br>
} else {<br>
// We're updating a function we've seen before.<br>
- Result = Dest.merge(I, Weight);<br>
+ Dest.merge(I, Weight);<br>
}<br>
<br>
Dest.sortValueData();<br>
<br>
- return Result;<br>
+ return Dest.getError();<br>
}<br>
<br>
bool InstrProfWriter::shouldEncodeData(const ProfilingData &PD) {<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>
</blockquote></div><br></div>
</div></blockquote></div><br></div></blockquote></div></div>