<div dir="ltr">Justin, any more comments? When this one goes in, Betul can send rest of the patches to be reviewed.<div><br></div><div>thanks,</div><div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 22, 2015 at 9:41 AM, Betul Buyukkurt <span dir="ltr"><<a href="mailto:betulb@codeaurora.org" target="_blank">betulb@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Ping?<br>
<br>
-----Original Message-----<br>
From: Betul Buyukkurt [mailto:<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>]<br>
</span><span class="">Sent: Friday, September 18, 2015 11:41 AM<br>
To: Justin Bogner<br>
Cc: Betul Buyukkurt; <a href="mailto:dnovillo@google.com">dnovillo@google.com</a>; <a href="mailto:dsule@codeaurora.org">dsule@codeaurora.org</a>;<br>
<a href="mailto:davidxl@google.com">davidxl@google.com</a>; <a href="mailto:ibaev@codeaurora.org">ibaev@codeaurora.org</a>;<br>
<a href="mailto:reviews%2Bd10674%2Bpublic%2B89cd3d23a57d43c9@reviews.llvm.org">reviews+d10674+public+89cd3d23a57d43c9@reviews.llvm.org</a>;<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
Subject: Re: [PATCH] D10674: Value profiling - patchset 3<br>
<br>
<br>
</span><div><div class="h5">Hi Justin,<br>
<br>
Thanks for the review comments. Please find my responses inline.<br>
<br>
Thanks,<br>
-Betul<br>
<br>
> Betul Buyukkurt <<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>> writes:<br>
>> betulb updated this revision to Diff 33963.<br>
>> betulb added a comment.<br>
>><br>
>> In this revision:<br>
>><br>
>> - Turned the error to assert for "hash value not matching any known key"<br>
>> - Used rvalue-reference semantics when passing arguments into the<br>
>> InstrProfWriter's addRecord routine. String table usage caused the<br>
>> undesirable removal of const qualifier from addRecord's argument. Now<br>
>> the arguments to addRecord are clearly passed using std::move()<br>
>> - In combineInstrProfRecords, tried to account for when value<br>
>> profiling is not enabled for a given kind for Source vs enabled for<br>
>> Dest and vice versa.<br>
>> - Used std::vector's empty() instead of comparing size() against 0<br>
><br>
> This is starting to look pretty good. I have a few more comments, but<br>
> first, there was one suggestion I made in a previous mail that I'm not<br>
> entirely convinced by your answer to:<br>
><br>
> Betul Buyukkurt <<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>> writes:<br>
>>>> /// Profiling information for a single function.<br>
>>>> struct InstrProfRecord {<br>
>>>> InstrProfRecord() {}<br>
>>>> InstrProfRecord(StringRef Name, uint64_t Hash,<br>
>>>> std::vector<uint64_t><br>
> Counts)<br>
>>>> : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}<br>
>>>> StringRef Name;<br>
>>>> uint64_t Hash;<br>
>>>> std::vector<uint64_t> Counts;<br>
>>>> + // Size of vector indicates the number of value sites for a<br>
</div></div>>>>> + value<br>
<div><div class="h5">> kind<br>
>>>> + std::vector<InstrProfValueSiteRecord><br>
> ValueSites[instrprof_value_kind::size];<br>
>>><br>
>>> I don't think we're gaining much by having this be an array - I was<br>
>>> thinking it would be more like<br>
>>><br>
>>> std::vector<InstrProfValueSiteRecord> IndirectCalls;<br>
<br>
Done.<br>
<br>
>>><br>
>>> Then when we add more value types, they can have their own variables<br>
> and<br>
>>> be accessed directly. Most of the code that works with these will<br>
>>> have<br>
> a<br>
>>> particular kind in mind, and since the value data is dependent on<br>
>>> kind looping over these isn't generally that useful. That is, the<br>
>>> looping we have now is only in the reader and writer, and I can't<br>
>>> see the users of the data ever doing that.<br>
>><br>
>> I think, I'm leaning towards keeping an array of kinds here.<br>
><br>
> Why?<br>
><br>
>>> For the reader and writer, a switch statement over the kinds will<br>
>>> allow us to warn if someone doesn't update somewhere when they add a<br>
>>> new kind. For the users of profile data, Data->IndirectCalls reads a<br>
>>> lot better than<br>
> Data->ValueSites[instrprof_value_kind::indirect_call_target].<br>
>><br>
>> I'll revisit my latest patch to allow for that.<br>
><br>
> Obviously, you didn't do this, since the comment above said you<br>
> weren't going to.<br>
><br>
> A few more things on the latest patch:<br>
><br>
> Betul Buyukkurt <<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>> writes:<br>
>> Index: unittests/ProfileData/InstrProfTest.cpp<br>
>> ===================================================================<br>
>> --- unittests/ProfileData/InstrProfTest.cpp<br>
>> +++ unittests/ProfileData/InstrProfTest.cpp<br>
>> @@ -50,7 +50,8 @@<br>
>> }<br>
>><br>
>> TEST_F(InstrProfTest, write_and_read_one_function) {<br>
>> - Writer.addFunctionCounts("foo", 0x1234, {1, 2, 3, 4});<br>
>> + InstrProfRecord Record("foo", 0x1234, {1, 2, 3, 4});<br>
>> + Writer.addRecord(std::move(Record));<br>
>> auto Profile = Writer.writeBuffer();<br>
>> readProfile(std::move(Profile));<br>
>><br>
>> @@ -67,8 +68,10 @@<br>
>> }<br>
>><br>
>> TEST_F(InstrProfTest, get_function_counts) {<br>
>> - Writer.addFunctionCounts("foo", 0x1234, {1, 2});<br>
>> - Writer.addFunctionCounts("foo", 0x1235, {3, 4});<br>
</div></div>>> + InstrProfRecord Record1("foo", 0x1234, {1, 2}); InstrProfRecord<br>
>> + Record2("foo", 0x1235, {3, 4});<br>
<span class="">>> + Writer.addRecord(std::move(Record1));<br>
>> + Writer.addRecord(std::move(Record2));<br>
>> auto Profile = Writer.writeBuffer();<br>
>> readProfile(std::move(Profile));<br>
>><br>
>> @@ -92,9 +95,12 @@<br>
>> }<br>
>><br>
>> TEST_F(InstrProfTest, get_max_function_count) {<br>
>> - Writer.addFunctionCounts("foo", 0x1234, {1ULL << 31, 2});<br>
>> - Writer.addFunctionCounts("bar", 0, {1ULL << 63});<br>
>> - Writer.addFunctionCounts("baz", 0x5678, {0, 0, 0, 0});<br>
>> + InstrProfRecord Record1("foo", 0x1234, {1ULL << 31, 2});<br>
</span>>> + InstrProfRecord Record2("bar", 0, {1ULL << 63}); InstrProfRecord<br>
>> + Record3("baz", 0x5678, {0, 0, 0, 0});<br>
<div><div class="h5">>> + Writer.addRecord(std::move(Record1));<br>
>> + Writer.addRecord(std::move(Record2));<br>
>> + Writer.addRecord(std::move(Record3));<br>
>> auto Profile = Writer.writeBuffer();<br>
>> readProfile(std::move(Profile));<br>
>><br>
>> Index: unittests/ProfileData/CoverageMappingTest.cpp<br>
>> ===================================================================<br>
>> --- unittests/ProfileData/CoverageMappingTest.cpp<br>
>> +++ unittests/ProfileData/CoverageMappingTest.cpp<br>
>> @@ -188,7 +188,8 @@<br>
>> }<br>
>><br>
>> TEST_F(CoverageMappingTest, basic_coverage_iteration) {<br>
>> - ProfileWriter.addFunctionCounts("func", 0x1234, {30, 20, 10, 0});<br>
>> + InstrProfRecord Record("func", 0x1234, {30, 20, 10, 0});<br>
>> + ProfileWriter.addRecord(std::move(Record));<br>
>> readProfCounts();<br>
>><br>
>> addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9); @@ -238,7<br>
>> +239,8 @@ }<br>
>><br>
>> TEST_F(CoverageMappingTest, combine_regions) {<br>
>> - ProfileWriter.addFunctionCounts("func", 0x1234, {10, 20, 30});<br>
>> + InstrProfRecord Record("func", 0x1234, {10, 20, 30});<br>
>> + ProfileWriter.addRecord(std::move(Record));<br>
>> readProfCounts();<br>
>><br>
>> addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9); @@ -256,7<br>
>> +258,8 @@ }<br>
>><br>
>> TEST_F(CoverageMappingTest, dont_combine_expansions) {<br>
>> - ProfileWriter.addFunctionCounts("func", 0x1234, {10, 20});<br>
>> + InstrProfRecord Record("func", 0x1234, {10, 20});<br>
>> + ProfileWriter.addRecord(std::move(Record));<br>
>> readProfCounts();<br>
>><br>
>> addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9); @@ -275,7<br>
>> +278,8 @@ }<br>
>><br>
>> TEST_F(CoverageMappingTest, strip_filename_prefix) {<br>
>> - ProfileWriter.addFunctionCounts("file1:func", 0x1234, {10});<br>
>> + InstrProfRecord Record("file1:func", 0x1234, {10});<br>
>> + ProfileWriter.addRecord(std::move(Record));<br>
>> readProfCounts();<br>
>><br>
>> addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);<br>
>> Index: tools/llvm-profdata/llvm-profdata.cpp<br>
>> ===================================================================<br>
>> --- tools/llvm-profdata/llvm-profdata.cpp<br>
>> +++ tools/llvm-profdata/llvm-profdata.cpp<br>
>> @@ -58,9 +58,8 @@<br>
>> exitWithError(ec.message(), Filename);<br>
>><br>
>> auto Reader = std::move(ReaderOrErr.get());<br>
>> - for (const auto &I : *Reader)<br>
>> - if (std::error_code EC =<br>
>> - Writer.addFunctionCounts(I.Name, I.Hash, I.Counts))<br>
>> + for (auto &I : *Reader)<br>
>> + if (std::error_code EC = Writer.addRecord(std::move(I)))<br>
>> errs() << Filename << ": " << I.Name << ": " << EC.message()<br>
>> <<<br>
> "\n";<br>
>> if (Reader->hasError())<br>
>> exitWithError(Reader->getError().message(), Filename); @@<br>
>> -134,8 +133,8 @@ }<br>
>><br>
>> static int showInstrProfile(std::string Filename, bool ShowCounts,<br>
>> - bool ShowAllFunctions, std::string<br>
> ShowFunction,<br>
>> - raw_fd_ostream &OS) {<br>
>> + bool ShowIndirectCallTargets, bool<br>
> ShowAllFunctions,<br>
>> + std::string ShowFunction, raw_fd_ostream<br>
> &OS) {<br>
>> auto ReaderOrErr = InstrProfReader::create(Filename);<br>
>> if (std::error_code EC = ReaderOrErr.getError())<br>
>> exitWithError(EC.message(), Filename); @@ -162,6 +161,10 @@<br>
>> << " Hash: " << format("0x%016" PRIx64, Func.Hash) << "\n"<br>
>> << " Counters: " << Func.Counts.size() << "\n"<br>
>> << " Function count: " << Func.Counts[0] << "\n";<br>
>> + if (ShowIndirectCallTargets)<br>
>> + OS << " Indirect Call Site Count: "<br>
>> + <<<br>
> Func.ValueSites[instrprof_value_kind::indirect_call_target].size()<br>
>> + << "\n";<br>
>> }<br>
>><br>
>> if (Show && ShowCounts)<br>
>> @@ -174,6 +177,17 @@<br>
>> }<br>
>> if (Show && ShowCounts)<br>
>> OS << "]\n";<br>
>> +<br>
>> + if (Show && ShowIndirectCallTargets) {<br>
>> + OS << " Indirect Target Results: \n";<br>
>> + uint32_t ValueKind = instrprof_value_kind::indirect_call_target;<br>
>> + for (size_t I = 0, E = Func.ValueSites[ValueKind].size(); I <<br>
</div></div>>> + E;<br>
<div class="HOEnZb"><div class="h5">> ++I) {<br>
>> + for (auto V : Func.ValueSites[ValueKind][I].ValueData) {<br>
>> + OS << "\t[ " << I << ", ";<br>
>> + OS << (const char *)V.first << ", " << V.second << " ]\n";<br>
>> + }<br>
>> + }<br>
>> + }<br>
>> }<br>
>> if (Reader->hasError())<br>
>> exitWithError(Reader->getError().message(), Filename); @@ -210,6<br>
>> +224,8 @@<br>
>><br>
>> cl::opt<bool> ShowCounts("counts", cl::init(false),<br>
>> cl::desc("Show counter values for shown<br>
> functions"));<br>
>> + cl::opt<bool> ShowIndirectCallTargets("ic-targets", cl::init(false),<br>
>> + cl::desc("Show indirect call site target values for shown<br>
> functions"));<br>
>> cl::opt<bool> ShowAllFunctions("all-functions", cl::init(false),<br>
>> cl::desc("Details for every<br>
> function"));<br>
>> cl::opt<std::string> ShowFunction("function", @@ -238,8 +254,8 @@<br>
>> errs() << "warning: -function argument ignored: showing all<br>
> functions\n";<br>
>><br>
>> if (ProfileKind == instr)<br>
>> - return showInstrProfile(Filename, ShowCounts, ShowAllFunctions,<br>
>> - ShowFunction, OS);<br>
>> + return showInstrProfile(Filename, ShowCounts,<br>
> ShowIndirectCallTargets,<br>
>> + ShowAllFunctions, ShowFunction, OS);<br>
>> else<br>
>> return showSampleProfile(Filename, ShowCounts, ShowAllFunctions,<br>
>> ShowFunction, OS);<br>
>> Index: lib/ProfileData/InstrProfWriter.cpp<br>
>> ===================================================================<br>
>> --- lib/ProfileData/InstrProfWriter.cpp<br>
>> +++ lib/ProfileData/InstrProfWriter.cpp<br>
>> @@ -26,8 +26,8 @@<br>
>> typedef StringRef key_type;<br>
>> typedef StringRef key_type_ref;<br>
>><br>
>> - typedef const InstrProfWriter::CounterData *const data_type;<br>
>> - typedef const InstrProfWriter::CounterData *const data_type_ref;<br>
>> + typedef const InstrProfWriter::ProfilingData *const data_type;<br>
>> + typedef const InstrProfWriter::ProfilingData *const data_type_ref;<br>
>><br>
>> typedef uint64_t hash_value_type;<br>
>> typedef uint64_t offset_type;<br>
>> @@ -45,8 +45,30 @@<br>
>> LE.write<offset_type>(N);<br>
>><br>
>> offset_type M = 0;<br>
>> - for (const auto &Counts : *V)<br>
>> - M += (2 + Counts.second.size()) * sizeof(uint64_t);<br>
>> + for (const auto &ProfileData : *V) {<br>
>> + M += sizeof(uint64_t); // size of function hash<br>
>> + M += sizeof(uint64_t); // size of<br>
> ProfileData.second.Counts.size()<br>
><br>
> I'd write this as "The function hash", and "The number of profile counts".<br>
<br>
Done. I used a different text for the latter suggestion.<br>
<br>
><br>
>> + M += ProfileData.second.Counts.size() * sizeof(uint64_t);<br>
>> +<br>
>> + // Value data<br>
>> + M += sizeof(uint64_t); // Number of value kinds with value sites.<br>
>> + for (uint32_t Kind = instrprof_value_kind::first;<br>
>> + Kind < instrprof_value_kind::size; ++Kind) {<br>
>> + if (ProfileData.second.ValueSites[Kind].empty())<br>
>> + continue;<br>
>> + M += sizeof(uint64_t); // Value kind<br>
>> + // Number of value sites for current value kind<br>
>> + M += sizeof(uint64_t); //<br>
> ProfileData.second.ValuesSites[Kind].size()<br>
><br>
> "The number of value kinds"<br>
<br>
Done.<br>
<br>
><br>
>> + for (InstrProfValueSiteRecord I :<br>
> ProfileData.second.ValueSites[Kind]) {<br>
>> + // Number of value data pairs at a value site<br>
>> + M += sizeof(uint64_t); // I.ValueData.size()<br>
>> + for (auto V : I.ValueData) {<br>
>> + M += sizeof(uint64_t); // size of TargetValue<br>
>> + M += sizeof(uint64_t); // size of NumTaken<br>
>> + }<br>
><br>
> Either way's fine, but would it read better to do<br>
><br>
> M += 2 * sizeof(uint64_t) * I.ValueData.size();<br>
><br>
> ?<br>
<br>
Done.<br>
<br>
<br>
>> + }<br>
>> + }<br>
>> + }<br>
>> LE.write<offset_type>(M);<br>
>><br>
>> return std::make_pair(N, M);<br>
>> @@ -60,52 +82,106 @@<br>
>> offset_type) {<br>
>> using namespace llvm::support;<br>
>> endian::Writer<little> LE(Out);<br>
>> -<br>
>> - for (const auto &Counts : *V) {<br>
>> - LE.write<uint64_t>(Counts.first);<br>
>> - LE.write<uint64_t>(Counts.second.size());<br>
>> - for (uint64_t I : Counts.second)<br>
>> + for (const auto &ProfileData : *V) {<br>
>> + LE.write<uint64_t>(ProfileData.first); // Function hash<br>
>> + LE.write<uint64_t>(ProfileData.second.Counts.size());<br>
>> + for (uint64_t I : ProfileData.second.Counts)<br>
>> LE.write<uint64_t>(I);<br>
>> +<br>
>> + // Compute the number of value kinds with value sites.<br>
>> + uint64_t NumValueKinds = 0;<br>
>> + for (uint32_t Kind = instrprof_value_kind::first;<br>
>> + Kind < instrprof_value_kind::size; ++Kind)<br>
>> + NumValueKinds +=<br>
> !(ProfileData.second.ValueSites[Kind].empty());<br>
>> + LE.write<uint64_t>(NumValueKinds);<br>
>> +<br>
>> + // Write value data<br>
>> + for (uint32_t Kind = instrprof_value_kind::first;<br>
>> + Kind < instrprof_value_kind::size; ++Kind) {<br>
>> + if (ProfileData.second.ValueSites[Kind].empty())<br>
>> + continue;<br>
>> + LE.write<uint64_t>(Kind); // Write value kind<br>
>> + // Write number of value sites for current value kind<br>
>> + LE.write<uint64_t>(ProfileData.second.ValueSites[Kind].size());<br>
>> + for (InstrProfValueSiteRecord I :<br>
> ProfileData.second.ValueSites[Kind]) {<br>
>> + // Write number of value data pairs at this value site<br>
>> + LE.write<uint64_t>(I.ValueData.size());<br>
>> + for (auto V : I.ValueData) {<br>
>> + if (Kind == instrprof_value_kind::indirect_call_target)<br>
>> + LE.write<uint64_t>(ComputeHash((const char *)V.first));<br>
>> + else<br>
>> + LE.write<uint64_t>(V.first);<br>
><br>
> This should just assert(Kind ==<br>
> instrprof_value_kind::indirect_call_target).<br>
> We don't want to be writing out data we don't understand. That said, if<br>
> you get rid of the array that doesn't come up.<br>
<br>
I'm doing the llvm_unreachable in InstrProf.h. I've defined two new<br>
methods for the InstrProfRecord. These are:<br>
const std::vector<InstrProfValueSiteRecord>& getValueSitesForKind(<br>
uint32_t ValueKind) const;<br>
and<br>
std::vector<InstrProfValueSiteRecord>& getValueSitesForKind(uint32_t<br>
ValueKind);.<br>
<br>
These methods use switch/case to pick the appropriate value site vector<br>
for the given value kind. If no value site vector was defined for the<br>
passed argument, the methods use llvm_unreachable instead of an explicit<br>
assert. Please check and let know if this still makes sense for you. To me<br>
this was a clean implementation.<br>
<br>
><br>
>> + LE.write<uint64_t>(V.second);<br>
>> + }<br>
>> + }<br>
>> + }<br>
>> }<br>
>> }<br>
>> };<br>
>> }<br>
>><br>
>> -std::error_code<br>
>> -InstrProfWriter::addFunctionCounts(StringRef FunctionName,<br>
>> - uint64_t FunctionHash,<br>
>> - ArrayRef<uint64_t> Counters) {<br>
>> - auto &CounterData = FunctionData[FunctionName];<br>
>> -<br>
>> - auto Where = CounterData.find(FunctionHash);<br>
>> - if (Where == CounterData.end()) {<br>
>> - // We've never seen a function with this name and hash, add it.<br>
>> - CounterData[FunctionHash] = Counters;<br>
>> - // We keep track of the max function count as we go for simplicity.<br>
>> - if (Counters[0] > MaxFunctionCount)<br>
>> - MaxFunctionCount = Counters[0];<br>
>> - return instrprof_error::success;<br>
>> - }<br>
>> -<br>
>> - // We're updating a function we've seen before.<br>
>> - auto &FoundCounters = Where->second;<br>
>> - // If the number of counters doesn't match we either have bad data or<br>
> a hash<br>
>> - // collision.<br>
>> - if (FoundCounters.size() != Counters.size())<br>
>> +static std::error_code combineInstrProfRecords(InstrProfRecord &Dest,<br>
>> + InstrProfRecord &Source,<br>
>> + uint64_t<br>
> &MaxFunctionCount) {<br>
>> + // If the number of counters doesn't match we either have bad data<br>
>> + // or a hash collision.<br>
>> + if (Dest.Counts.size() != Source.Counts.size())<br>
>> return instrprof_error::count_mismatch;<br>
>><br>
>> - for (size_t I = 0, E = Counters.size(); I < E; ++I) {<br>
>> - if (FoundCounters[I] + Counters[I] < FoundCounters[I])<br>
>> + for (size_t I = 0, E = Source.Counts.size(); I < E; ++I) {<br>
>> + if (Dest.Counts[I] + Source.Counts[I] < Dest.Counts[I])<br>
>> return instrprof_error::counter_overflow;<br>
>> - FoundCounters[I] += Counters[I];<br>
>> + Dest.Counts[I] += Source.Counts[I];<br>
>> + }<br>
>> +<br>
>> + for (uint32_t Kind = instrprof_value_kind::first;<br>
>> + Kind < instrprof_value_kind::size; ++Kind) {<br>
>> + if (Source.ValueSites[Kind].empty())<br>
>> + continue;<br>
>> + if (Dest.ValueSites[Kind].empty()) {<br>
>> + Dest.ValueSites[Kind].swap(Source.ValueSites[Kind]);<br>
>> + continue;<br>
>> + }<br>
><br>
> Why do we allow combining data that has no value sites with data that<br>
> does? Does that actually make sense?<br>
><br>
>> + if (Dest.ValueSites[Kind].size() != Source.ValueSites[Kind].size())<br>
>> + return instrprof_error::value_site_count_mismatch;<br>
>> + for (size_t I = 0, E = Source.ValueSites[Kind].size(); I < E; ++I)<br>
>> +<br>
> Dest.ValueSites[Kind][I].mergeValueData(Source.ValueSites[Kind][I]);<br>
>> }<br>
>> +<br>
>> // We keep track of the max function count as we go for simplicity.<br>
>> - if (FoundCounters[0] > MaxFunctionCount)<br>
>> - MaxFunctionCount = FoundCounters[0];<br>
>> + if (Dest.Counts[0] > MaxFunctionCount)<br>
>> + MaxFunctionCount = Dest.Counts[0];<br>
>><br>
>> return instrprof_error::success;<br>
>> }<br>
>><br>
>> +void InstrProfWriter::updateStringTableReferences(InstrProfRecord &I) {<br>
>> + I.Name = StringTable.insertString(I.Name);<br>
>> + for (auto& VSite :<br>
> I.ValueSites[instrprof_value_kind::indirect_call_target])<br>
>> + for (auto& VData : VSite.ValueData)<br>
>> + VData.first =<br>
>> + (uint64_t)StringTable.insertString((const char<br>
> *)VData.first);<br>
><br>
> I'm a little uncomfortable that we need to modify the Name in the<br>
> iterators here. Can't the data structure backing the string table just<br>
> work in StringRefs instead?<br>
><br>
>> +}<br>
>> +<br>
>> +std::error_code InstrProfWriter::addRecord(InstrProfRecord &&I) {<br>
>> + updateStringTableReferences(I);<br>
>> + auto &ProfileDataMap = FunctionData[I.Name];<br>
>> +<br>
>> + auto Where = ProfileDataMap.find(I.Hash);<br>
>> + if (Where == ProfileDataMap.end()) {<br>
>> + // We've never seen a function with this name and hash, add it.<br>
>> + ProfileDataMap[I.Hash] = I;<br>
>> +<br>
>> + // We keep track of the max function count as we go for simplicity.<br>
>> + if (I.Counts[0] > MaxFunctionCount)<br>
>> + MaxFunctionCount = I.Counts[0];<br>
>> + return instrprof_error::success;<br>
>> + }<br>
>> +<br>
>> + // We're updating a function we've seen before.<br>
>> + return combineInstrProfRecords(Where->second, I, MaxFunctionCount);<br>
>> +}<br>
>> +<br>
>> std::pair<uint64_t, uint64_t> InstrProfWriter::writeImpl(raw_ostream<br>
> &OS) {<br>
>> OnDiskChainedHashTableGenerator<InstrProfRecordTrait> Generator;<br>
>><br>
>> Index: lib/ProfileData/InstrProfReader.cpp<br>
>> ===================================================================<br>
>> --- lib/ProfileData/InstrProfReader.cpp<br>
>> +++ lib/ProfileData/InstrProfReader.cpp<br>
>> @@ -15,8 +15,12 @@<br>
>> #include "llvm/ProfileData/InstrProfReader.h"<br>
>> #include "InstrProfIndexed.h"<br>
>> #include "llvm/ADT/STLExtras.h"<br>
>> +#include "llvm/Support/Debug.h"<br>
>> +#include "llvm/Support/raw_ostream.h"<br>
>> #include <cassert><br>
>><br>
>> +#define DEBUG_TYPE "InstrProfReader"<br>
>> +<br>
>> using namespace llvm;<br>
>><br>
>> static ErrorOr<std::unique_ptr<MemoryBuffer>><br>
>> @@ -302,42 +306,94 @@<br>
>> typedef InstrProfLookupTrait::data_type data_type;<br>
>> typedef InstrProfLookupTrait::offset_type offset_type;<br>
>><br>
>> +bool InstrProfLookupTrait::ReadValueProfilingData(<br>
>> + const unsigned char *&D, const unsigned char *const End) {<br>
>> +<br>
>> + using namespace support;<br>
>> + // Read number of value kinds with value sites.<br>
>> + if (D + sizeof(uint64_t) > End)<br>
>> + return false;<br>
>> + uint64_t ValueKindCount = endian::readNext<uint64_t, little,<br>
> unaligned>(D);<br>
>> +<br>
>> + for (uint32_t Kind = 0; Kind < ValueKindCount; ++Kind) {<br>
>> +<br>
>> + // Read value kind and number of value sites for kind.<br>
>> + if (D + 2*sizeof(uint64_t) > End)<br>
>> + return false;<br>
>> + uint64_t ValueKind = endian::readNext<uint64_t, little,<br>
> unaligned>(D);<br>
>> + uint64_t ValueSiteCount = endian::readNext<uint64_t, little,<br>
> unaligned>(D);<br>
>> +<br>
>> + DataBuffer.back().ValueSites[ValueKind].reserve(ValueSiteCount);<br>
>> + for (uint64_t VSite = 0; VSite < ValueSiteCount; ++VSite) {<br>
>> + // Read number of value data pairs at value site.<br>
>> + if (D + sizeof(uint64_t) > End)<br>
>> + return false;<br>
>> + uint64_t ValueDataCount =<br>
>> + endian::readNext<uint64_t, little, unaligned>(D);<br>
>> +<br>
>> + // Check if there are as many ValueDataPairs as ValueDataCount in<br>
> memory.<br>
>> + if (D + (ValueDataCount<<1)*sizeof(uint64_t) > End)<br>
>> + return false;<br>
>> +<br>
>> + InstrProfValueSiteRecord VSiteRecord;<br>
>> + for (uint64_t VCount = 0; VCount < ValueDataCount; ++VCount) {<br>
>> + uint64_t Value = endian::readNext<uint64_t, little,<br>
> unaligned>(D);<br>
>> + uint64_t NumTaken = endian::readNext<uint64_t, little,<br>
> unaligned>(D);<br>
>> + if (ValueKind == instrprof_value_kind::indirect_call_target) {<br>
><br>
> If we see anything other than indirect_call_target we should return<br>
> false - we don't know how to interpret that data.<br>
<br>
The assert is placed in the getValueSitesForKind method.<br>
<br>
><br>
>> + auto Result = HashKeyMap.find(Value);<br>
>> + assert(Result != HashKeyMap.end() &&<br>
>> + "Hash does not match any known keys\n");<br>
>> + Value = (uint64_t)Result->second;<br>
>> + }<br>
>> + VSiteRecord.ValueData.push_back(std::make_pair(Value,<br>
> NumTaken));<br>
>> + }<br>
>> +<br>
> DataBuffer.back().ValueSites[ValueKind].push_back(std::move(VSiteRecord));<br>
>> + }<br>
>> + }<br>
>> + return true;<br>
>> +}<br>
>> +<br>
>> data_type InstrProfLookupTrait::ReadData(StringRef K, const unsigned<br>
> char *D,<br>
>> offset_type N) {<br>
>> -<br>
>> // Check if the data is corrupt. If so, don't try to read it.<br>
>> if (N % sizeof(uint64_t))<br>
>> return data_type();<br>
>><br>
>> DataBuffer.clear();<br>
>> - uint64_t NumCounts;<br>
>> - uint64_t NumEntries = N / sizeof(uint64_t);<br>
>> std::vector<uint64_t> CounterBuffer;<br>
>> - for (uint64_t I = 0; I < NumEntries; I += NumCounts) {<br>
>> - using namespace support;<br>
>> - // The function hash comes first.<br>
>> - uint64_t Hash = endian::readNext<uint64_t, little, unaligned>(D);<br>
>><br>
>> - if (++I >= NumEntries)<br>
>> + using namespace support;<br>
>> + const unsigned char *End = D + N;<br>
>> + while (D < End) {<br>
>> + // Read hash<br>
>> + if (D + sizeof(uint64_t) >= End)<br>
>> return data_type();<br>
>> + uint64_t Hash = endian::readNext<uint64_t, little, unaligned>(D);<br>
>><br>
>> - // In v1, we have at least one count.<br>
>> - // Later, we have the number of counts.<br>
>> - NumCounts = (1 == FormatVersion)<br>
>> - ? NumEntries - I<br>
>> - : endian::readNext<uint64_t, little, unaligned>(D);<br>
>> - if (1 != FormatVersion)<br>
>> - ++I;<br>
>> -<br>
>> - // If we have more counts than data, this is bogus.<br>
>> - if (I + NumCounts > NumEntries)<br>
>> + // Initialize number of counters for FormatVersion == 1<br>
>> + uint64_t CountsSize = N / sizeof(uint64_t) - 1;<br>
>> + // If format version is different then read number of counters<br>
>> + if (FormatVersion != 1) {<br>
>> + if (D + sizeof(uint64_t) > End)<br>
>> + return data_type();<br>
>> + CountsSize = endian::readNext<uint64_t, little, unaligned>(D);<br>
>> + }<br>
>> + // Read counter values<br>
>> + if (D + CountsSize * sizeof(uint64_t) > End)<br>
>> return data_type();<br>
>><br>
>> CounterBuffer.clear();<br>
>> - for (unsigned J = 0; J < NumCounts; ++J)<br>
>> + CounterBuffer.reserve(CountsSize);<br>
>> + for (uint64_t J = 0; J < CountsSize; ++J)<br>
>> CounterBuffer.push_back(endian::readNext<uint64_t, little,<br>
> unaligned>(D));<br>
>><br>
>> DataBuffer.push_back(InstrProfRecord(K, Hash,<br>
> std::move(CounterBuffer)));<br>
>> +<br>
>> + // Read value profiling data<br>
>> + if (FormatVersion == 3 && !ReadValueProfilingData(D, End)) {<br>
><br>
> This should be "FormatVersion > 2".<br>
<br>
Done.<br>
<br>
><br>
>> + DataBuffer.clear();<br>
>> + return data_type();<br>
>> + }<br>
>> }<br>
>> return DataBuffer;<br>
>> }<br>
>> @@ -383,7 +439,19 @@<br>
>> // The rest of the file is an on disk hash table.<br>
>> Index.reset(InstrProfReaderIndex::Create(<br>
>> Start + HashOffset, Cur, Start,<br>
>> - InstrProfLookupTrait(HashType, FormatVersion)));<br>
>> + InstrProfLookupTrait(HashType, FormatVersion, HashKeyMap)));<br>
>> + for (auto Key : Index->keys()) {<br>
>> + const char *KeyTableRef = StringTable.insertString(Key);<br>
>> + auto Result =<br>
> HashKeyMap.insert(std::make_pair(ComputeHash(HashType, Key),<br>
>> + KeyTableRef));<br>
><br>
> The way the HashKeyMap works here seems a little off. Why does the<br>
> Reader own it? Only the trait uses it. Also, since it follows a strict<br>
> "fill then query" pattern, it's probably better to just use a vector<br>
> that we sort after filling and then binary search later.<br>
<br>
Done.<br>
<br>
><br>
>> + // Emit warning if a hash collision is detected.<br>
>> + if (Result.second == false)<br>
>> + DEBUG(dbgs() << "IndexedInstrProfReader: hash collision detected:<br>
> \n"<br>
>> + << "\t Map Entry(Hash, Key): " <<<br>
> Result.first->first<br>
>> + << ", " << Result.first->second << "\n"<br>
>> + << "\t New Entry(Hash, Key): " <<<br>
> ComputeHash(HashType, Key)<br>
>> + << ", " << Key << "\n");<br>
><br>
> This is not "emitting a warning". This will only be printed if the host<br>
> compiler is built in debug mode, so it seems pretty pointless. Actually<br>
> emitting a proper warning from this point in the compiler might be kind<br>
> of tricky though.<br>
><br>
>> + }<br>
>> // Set up our iterator for readNextRecord.<br>
>> RecordIterator = Index->data_begin();<br>
>><br>
>> Index: lib/ProfileData/InstrProfIndexed.h<br>
>> ===================================================================<br>
>> --- lib/ProfileData/InstrProfIndexed.h<br>
>> +++ lib/ProfileData/InstrProfIndexed.h<br>
>> @@ -47,7 +47,7 @@<br>
>> }<br>
>><br>
>> const uint64_t Magic = 0x8169666f72706cff; // "\xfflprofi\x81"<br>
>> -const uint64_t Version = 2;<br>
>> +const uint64_t Version = 3;<br>
>> const HashT HashType = HashT::MD5;<br>
>> }<br>
>><br>
>> Index: lib/ProfileData/InstrProf.cpp<br>
>> ===================================================================<br>
>> --- lib/ProfileData/InstrProf.cpp<br>
>> +++ lib/ProfileData/InstrProf.cpp<br>
>> @@ -50,6 +50,8 @@<br>
>> return "Function count mismatch";<br>
>> case instrprof_error::counter_overflow:<br>
>> return "Counter overflow";<br>
>> + case instrprof_error::value_site_count_mismatch:<br>
>> + return "Function's value site counts mismatch";<br>
>> }<br>
>> llvm_unreachable("A value of instrprof_error has no message.");<br>
>> }<br>
>> Index: include/llvm/ProfileData/InstrProfWriter.h<br>
>> ===================================================================<br>
>> --- include/llvm/ProfileData/InstrProfWriter.h<br>
>> +++ include/llvm/ProfileData/InstrProfWriter.h<br>
>> @@ -15,33 +15,32 @@<br>
>> #ifndef LLVM_PROFILEDATA_INSTRPROFWRITER_H<br>
>> #define LLVM_PROFILEDATA_INSTRPROFWRITER_H<br>
>><br>
>> -#include "llvm/ADT/ArrayRef.h"<br>
>> #include "llvm/ADT/DenseMap.h"<br>
>> -#include "llvm/ADT/StringMap.h"<br>
>> #include "llvm/ProfileData/InstrProf.h"<br>
>> #include "llvm/Support/DataTypes.h"<br>
>> #include "llvm/Support/MemoryBuffer.h"<br>
>> #include "llvm/Support/raw_ostream.h"<br>
>> -#include <vector><br>
>><br>
>> namespace llvm {<br>
>><br>
>> /// Writer for instrumentation based profile data.<br>
>> class InstrProfWriter {<br>
>> public:<br>
>> - typedef SmallDenseMap<uint64_t, std::vector<uint64_t>, 1><br>
> CounterData;<br>
>> + typedef SmallDenseMap<uint64_t, InstrProfRecord, 1> ProfilingData;<br>
>> +<br>
>> private:<br>
>> - StringMap<CounterData> FunctionData;<br>
>> + InstrProfStringTable StringTable;<br>
>> + StringMap<ProfilingData> FunctionData;<br>
>> uint64_t MaxFunctionCount;<br>
>> public:<br>
>> InstrProfWriter() : MaxFunctionCount(0) {}<br>
>><br>
>> + /// Update string entries in profile data with references to<br>
> StringTable.<br>
>> + void updateStringTableReferences(InstrProfRecord &I);<br>
>> /// Add function counts for the given function. If there are already<br>
> counts<br>
>> /// for this function and the hash and number of counts match, each<br>
> counter is<br>
>> /// summed.<br>
>> - std::error_code addFunctionCounts(StringRef FunctionName,<br>
>> - uint64_t FunctionHash,<br>
>> - ArrayRef<uint64_t> Counters);<br>
>> + std::error_code addRecord(InstrProfRecord &&I);<br>
>> /// Write the profile to \c OS<br>
>> void write(raw_fd_ostream &OS);<br>
>> /// Write the profile, returning the raw data. For testing.<br>
>> Index: include/llvm/ProfileData/InstrProfReader.h<br>
>> ===================================================================<br>
>> --- include/llvm/ProfileData/InstrProfReader.h<br>
>> +++ include/llvm/ProfileData/InstrProfReader.h<br>
>> @@ -24,6 +24,7 @@<br>
>> #include "llvm/Support/MemoryBuffer.h"<br>
>> #include "llvm/Support/OnDiskHashTable.h"<br>
>> #include <iterator><br>
>> +#include <map><br>
>><br>
>> namespace llvm {<br>
>><br>
>> @@ -65,6 +66,9 @@<br>
>> InstrProfIterator end() { return InstrProfIterator(); }<br>
>><br>
>> protected:<br>
>> + /// String table for holding a unique copy of all the strings in the<br>
> profile.<br>
>> + InstrProfStringTable StringTable;<br>
>> +<br>
>> /// Set the current std::error_code and return same.<br>
>> std::error_code error(std::error_code EC) {<br>
>> LastError = EC;<br>
>> @@ -195,10 +199,13 @@<br>
>> std::vector<InstrProfRecord> DataBuffer;<br>
>> IndexedInstrProf::HashT HashType;<br>
>> unsigned FormatVersion;<br>
>> + const std::map<uint64_t, const char *> &HashKeyMap;<br>
>><br>
>> public:<br>
>> - InstrProfLookupTrait(IndexedInstrProf::HashT HashType, unsigned<br>
> FormatVersion)<br>
>> - : HashType(HashType), FormatVersion(FormatVersion) {}<br>
>> + InstrProfLookupTrait(IndexedInstrProf::HashT HashType, unsigned<br>
> FormatVersion,<br>
>> + std::map<uint64_t, const char *> &HashKeyMap)<br>
>> + : HashType(HashType), FormatVersion(FormatVersion),<br>
>> + HashKeyMap(HashKeyMap) {}<br>
>><br>
>> typedef ArrayRef<InstrProfRecord> data_type;<br>
>><br>
>> @@ -209,6 +216,7 @@<br>
>><br>
>> static bool EqualKey(StringRef A, StringRef B) { return A == B; }<br>
>> static StringRef GetInternalKey(StringRef K) { return K; }<br>
>> + static StringRef GetExternalKey(StringRef K) { return K; }<br>
><br>
> What do you need GetExternalKey for?<br>
<br>
If you want to be able to iterate over the keys of the<br>
OnDiskChainedIterableHashTable, then you have to have this method defined.<br>
We're iterating over the keys in the reader to form the HashKeyMap, so<br>
this had to be defined.<br>
<br>
><br>
>><br>
>> hash_value_type ComputeHash(StringRef K);<br>
>><br>
>> @@ -224,6 +232,8 @@<br>
>> return StringRef((const char *)D, N);<br>
>> }<br>
>><br>
>> + bool ReadValueProfilingData(const unsigned char *&D,<br>
>> + const unsigned char *const End);<br>
>> data_type ReadData(StringRef K, const unsigned char *D, offset_type<br>
> N);<br>
>> };<br>
>><br>
>> @@ -243,6 +253,8 @@<br>
>> uint64_t FormatVersion;<br>
>> /// The maximal execution count among all functions.<br>
>> uint64_t MaxFunctionCount;<br>
>> + /// Map of hash values to const char* keys in profiling data.<br>
>> + std::map<uint64_t, const char *> HashKeyMap;<br>
>><br>
>> IndexedInstrProfReader(const IndexedInstrProfReader &) = delete;<br>
>> IndexedInstrProfReader &operator=(const IndexedInstrProfReader &) =<br>
> delete;<br>
>> Index: include/llvm/ProfileData/InstrProf.h<br>
>> ===================================================================<br>
>> --- include/llvm/ProfileData/InstrProf.h<br>
>> +++ include/llvm/ProfileData/InstrProf.h<br>
>> @@ -16,42 +16,102 @@<br>
>> #ifndef LLVM_PROFILEDATA_INSTRPROF_H_<br>
>> #define LLVM_PROFILEDATA_INSTRPROF_H_<br>
>><br>
>> -#include "llvm/ADT/StringRef.h"<br>
>> +#include "llvm/ADT/StringSet.h"<br>
>> #include <cstdint><br>
>> +#include <list><br>
>> #include <system_error><br>
>> #include <vector><br>
>><br>
>> namespace llvm {<br>
>> const std::error_category &instrprof_category();<br>
>><br>
>> enum class instrprof_error {<br>
>> - success = 0,<br>
>> - eof,<br>
>> - bad_magic,<br>
>> - bad_header,<br>
>> - unsupported_version,<br>
>> - unsupported_hash_type,<br>
>> - too_large,<br>
>> - truncated,<br>
>> - malformed,<br>
>> - unknown_function,<br>
>> - hash_mismatch,<br>
>> - count_mismatch,<br>
>> - counter_overflow<br>
>> + success = 0,<br>
>> + eof,<br>
>> + bad_magic,<br>
>> + bad_header,<br>
>> + unsupported_version,<br>
>> + unsupported_hash_type,<br>
>> + too_large,<br>
>> + truncated,<br>
>> + malformed,<br>
>> + unknown_function,<br>
>> + hash_mismatch,<br>
>> + count_mismatch,<br>
>> + counter_overflow,<br>
>> + value_site_count_mismatch<br>
>> };<br>
>><br>
>> inline std::error_code make_error_code(instrprof_error E) {<br>
>> return std::error_code(static_cast<int>(E), instrprof_category());<br>
>> }<br>
>><br>
>> +enum instrprof_value_kind : uint32_t {<br>
>> + first = 0,<br>
>> + indirect_call_target = 0,<br>
>> + size = 1<br>
>> +};<br>
>> +<br>
>> +struct InstrProfStringTable {<br>
>> + // Set of string values in profiling data.<br>
>> + StringSet<> StringValueSet;<br>
>> + InstrProfStringTable() { StringValueSet.clear(); }<br>
>> + // Get a pointer to internal storage of a string in set<br>
>> + const char *getStringData(StringRef Str) {<br>
>> + auto Result = StringValueSet.find(Str);<br>
>> + return (Result == StringValueSet.end()) ? nullptr :<br>
> Result->first().data();<br>
>> + }<br>
>> + // Insert a string to StringTable<br>
>> + const char *insertString(StringRef Str) {<br>
>> + auto Result = StringValueSet.insert(Str);<br>
>> + return Result.first->first().data();<br>
>> + }<br>
>> +};<br>
>> +<br>
>> +struct InstrProfValueSiteRecord {<br>
>> + // Typedef for a single TargetValue-NumTaken pair.<br>
>> + typedef std::pair<uint64_t, uint64_t> ValueDataPair;<br>
>> + // Value profiling data pairs at a given value site.<br>
>> + std::list<ValueDataPair> ValueData;<br>
>> +<br>
>> + InstrProfValueSiteRecord() { ValueData.clear(); }<br>
>> +<br>
>> + // Sort ValueData ascending by TargetValue<br>
>> + void sortByTargetValues() {<br>
>> + ValueData.sort([](const ValueDataPair &left, const ValueDataPair<br>
> &right) {<br>
>> + return left.first < right.first;<br>
>> + });<br>
>> + }<br>
>> + // Merge data from another InstrProfValueSiteRecord<br>
>> + void mergeValueData(InstrProfValueSiteRecord &Input) {<br>
>> + this->sortByTargetValues();<br>
>> + Input.sortByTargetValues();<br>
>> + auto I = ValueData.begin();<br>
>> + auto IE = ValueData.end();<br>
>> + for (auto J = Input.ValueData.begin(), JE = Input.ValueData.end();<br>
> J != JE;<br>
>> + ++J) {<br>
>> + while (I != IE && I->first < J->first)<br>
>> + ++I;<br>
>> + if (I != IE && I->first == J->first) {<br>
>> + I->second += J->second;<br>
>> + ++I;<br>
>> + continue;<br>
>> + }<br>
>> + ValueData.insert(I, *J);<br>
>> + }<br>
>> + }<br>
>> +};<br>
>> +<br>
>> /// Profiling information for a single function.<br>
>> struct InstrProfRecord {<br>
>> InstrProfRecord() {}<br>
>> InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t><br>
> Counts)<br>
>> : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}<br>
>> StringRef Name;<br>
>> uint64_t Hash;<br>
>> std::vector<uint64_t> Counts;<br>
>> + // Size of vector indicates the number of value sites for a value<br>
> kind<br>
>> + std::vector<InstrProfValueSiteRecord><br>
> ValueSites[instrprof_value_kind::size];<br>
>> };<br>
>><br>
>> } // end namespace llvm<br>
><br>
><br>
<br>
<br>
</div></div></blockquote></div><br></div>