<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jun 20, 2015 at 8:04 PM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">><br>
><br>
> On Sat, Jun 20, 2015 at 3:15 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a><br>
</span>> <mailto:<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> > wrote:<br>
><br>
><br>
> Betul Buyukkurt <<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a> <mailto:<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>> ><br>
<div><div class="h5">> writes:<br>
>> Included <cstdint> to InstrProf.h<br>
>><br>
>><br>
>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10493&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=dcMXkHodfDkGhshv6bqpkKZ6oACqR-9DB9OQrQlFS6I&s=Ccj6TayB_lEaCmZGXqjqEKozNwVo0Z5UsrxljcOHgwg&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10493</a><br>
>><br>
>> Files:<br>
>> include/llvm/ProfileData/InstrProf.h<br>
>> include/llvm/ProfileData/InstrProfReader.h<br>
>> lib/ProfileData/InstrProfReader.cpp<br>
>><br>
>> EMAIL PREFERENCES<br>
>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=dcMXkHodfDkGhshv6bqpkKZ6oACqR-9DB9OQrQlFS6I&s=5uehHU-TGFTCddqj-2yayCGs0MOm7vBRaq_QWsltjvA&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
><br>
>> Index: include/llvm/ProfileData/InstrProf.h<br>
>> ===================================================================<br>
>> --- include/llvm/ProfileData/InstrProf.h<br>
>> +++ include/llvm/ProfileData/InstrProf.h<br>
>> @@ -16,7 +16,10 @@<br>
>> #ifndef LLVM_PROFILEDATA_INSTRPROF_H_<br>
>> #define LLVM_PROFILEDATA_INSTRPROF_H_<br>
>><br>
>> +#include "llvm/ADT/StringRef.h"<br>
>> +#include <cstdint><br>
>> #include <system_error><br>
>> +#include <vector><br>
>><br>
>> namespace llvm {<br>
>> const std::error_category &instrprof_category();<br>
>> @@ -41,6 +44,16 @@<br>
>> return std::error_code(static_cast<int>(E), instrprof_category());<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>
><br>
> Please use an rvalue reference for Counts.<br>
><br>
><br>
><br>
> Generally pass-by-value's the right tool in my experience. That way the<br>
> caller can decide whether to move into the operand or copy, depending on<br>
> their needs. The callee doesn't have to force the caller one way or<br>
> another.<br>
<br>
</div></div>I totally agree. I did not understand what was meant by pass by value in<br>
the previous comment. I now understood the concern. I fixed it in my<br>
latest patch. Thanks for the comments.<br></blockquote><div><br>Looks like your current version is passing by rvalue reference:<br><br><div>+ InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t> &&Counts)</div><div>+ : Name(Name), Hash(Hash), Counts(Counts) {}<br><br>What I was suggesting was to pass by value:<br><br><div>+ InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t> Counts)</div><div>+ : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}<br><br>Works just as well, but lets callers /not/ pass ownership if they don't want to.<br><br>Oh, and actually you forgot the std::move in the ctor init list (perhaps that's what you thought I meant by pass by value? Sorry for the confusion/lack of clarity)</div></div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
-Betul<br>
<div><div class="h5"><br>
><br>
><br>
>> + : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}<br>
>> + StringRef Name;<br>
>> + uint64_t Hash;<br>
>> + std::vector<uint64_t> Counts;<br>
>> +};<br>
>> +<br>
>> } // end namespace llvm<br>
>><br>
>> namespace std {<br>
>> Index: include/llvm/ProfileData/InstrProfReader.h<br>
>> ===================================================================<br>
>> --- include/llvm/ProfileData/InstrProfReader.h<br>
>> +++ include/llvm/ProfileData/InstrProfReader.h<br>
>> @@ -29,16 +29,6 @@<br>
>><br>
>> class InstrProfReader;<br>
>><br>
>> -/// Profiling information for a single function.<br>
>> -struct InstrProfRecord {<br>
>> - InstrProfRecord() {}<br>
>> - InstrProfRecord(StringRef Name, uint64_t Hash, ArrayRef<uint64_t><br>
>> Counts)<br>
>> - : Name(Name), Hash(Hash), Counts(Counts) {}<br>
>> - StringRef Name;<br>
>> - uint64_t Hash;<br>
>> - ArrayRef<uint64_t> Counts;<br>
>> -};<br>
>> -<br>
>> /// A file format agnostic iterator over profiling data.<br>
>> class InstrProfIterator : public std::iterator<std::input_iterator_tag,<br>
>> InstrProfRecord> {<br>
>> @@ -114,8 +104,6 @@<br>
>> std::unique_ptr<MemoryBuffer> DataBuffer;<br>
>> /// Iterator over the profile data.<br>
>> line_iterator Line;<br>
>> - /// The current set of counter values.<br>
>> - std::vector<uint64_t> Counts;<br>
>><br>
>> TextInstrProfReader(const TextInstrProfReader &) = delete;<br>
>> TextInstrProfReader &operator=(const TextInstrProfReader &) = delete;<br>
>> @@ -141,8 +129,6 @@<br>
>> private:<br>
>> /// The profile data file contents.<br>
>> std::unique_ptr<MemoryBuffer> DataBuffer;<br>
>> - /// The current set of counter values.<br>
>> - std::vector<uint64_t> Counts;<br>
>> struct ProfileData {<br>
>> const uint32_t NameSize;<br>
>> const uint32_t NumCounters;<br>
><br>
>> @@ -206,17 +192,16 @@<br>
>> /// Trait for lookups into the on-disk hash table for the binary<br>
>> instrprof<br>
>> /// format.<br>
>> class InstrProfLookupTrait {<br>
>> - std::vector<uint64_t> DataBuffer;<br>
>> + std::vector<InstrProfRecord> DataBuffer;<br>
>> IndexedInstrProf::HashT HashType;<br>
>> + unsigned FormatVersion;<br>
>> +<br>
>> public:<br>
>> - InstrProfLookupTrait(IndexedInstrProf::HashT HashType) :<br>
>> HashType(HashType) {}<br>
>> + InstrProfLookupTrait(IndexedInstrProf::HashT HashType, unsigned<br>
>> FormatVersion)<br>
>> + : HashType(HashType), FormatVersion(FormatVersion) {}<br>
>> +<br>
>> + typedef ArrayRef<InstrProfRecord> data_type;<br>
>><br>
>> - struct data_type {<br>
>> - data_type(StringRef Name, ArrayRef<uint64_t> Data)<br>
>> - : Name(Name), Data(Data) {}<br>
>> - StringRef Name;<br>
>> - ArrayRef<uint64_t> Data;<br>
>> - };<br>
>> typedef StringRef internal_key_type;<br>
>> typedef StringRef external_key_type;<br>
>> typedef uint64_t hash_value_type;<br>
>> @@ -239,22 +224,9 @@<br>
>> return StringRef((const char *)D, N);<br>
>> }<br>
>><br>
>> - data_type ReadData(StringRef K, const unsigned char *D, offset_type<br>
>> N)<br>
>> {<br>
>> - DataBuffer.clear();<br>
>> - if (N % sizeof(uint64_t))<br>
>> - // The data is corrupt, don't try to read it.<br>
>> - return data_type("", DataBuffer);<br>
>> -<br>
>> - using namespace support;<br>
>> - // We just treat the data as opaque here. It's simpler to handle in<br>
>> - // IndexedInstrProfReader.<br>
>> - unsigned NumEntries = N / sizeof(uint64_t);<br>
>> - DataBuffer.reserve(NumEntries);<br>
>> - for (unsigned I = 0; I < NumEntries; ++I)<br>
>> - DataBuffer.push_back(endian::readNext<uint64_t, little,<br>
>> unaligned>(D));<br>
>> - return data_type(K, DataBuffer);<br>
>> - }<br>
>> + data_type ReadData(StringRef K, const unsigned char *D, offset_type<br>
>> N);<br>
>> };<br>
>> +<br>
>> typedef OnDiskIterableChainedHashTable<InstrProfLookupTrait><br>
>> InstrProfReaderIndex;<br>
>><br>
>> @@ -267,8 +239,6 @@<br>
>> std::unique_ptr<InstrProfReaderIndex> Index;<br>
>> /// Iterator over the profile data.<br>
>> InstrProfReaderIndex::data_iterator RecordIterator;<br>
>> - /// Offset into our current data set.<br>
>> - size_t CurrentOffset;<br>
>> /// The file format version of the profile data.<br>
>> uint64_t FormatVersion;<br>
>> /// The maximal execution count among all functions.<br>
>> @@ -278,7 +248,7 @@<br>
><br>
>> IndexedInstrProfReader &operator=(const IndexedInstrProfReader &) =<br>
>> delete;<br>
>> public:<br>
>> IndexedInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer)<br>
>> - : DataBuffer(std::move(DataBuffer)), Index(nullptr),<br>
>> CurrentOffset(0) {}<br>
>> + : DataBuffer(std::move(DataBuffer)), Index(nullptr) {}<br>
>><br>
>> /// Return true if the given buffer is in an indexed instrprof<br>
>> format.<br>
>> static bool hasFormat(const MemoryBuffer &DataBuffer);<br>
>> Index: lib/ProfileData/InstrProfReader.cpp<br>
>> ===================================================================<br>
>> --- lib/ProfileData/InstrProfReader.cpp<br>
>> +++ lib/ProfileData/InstrProfReader.cpp<br>
>> @@ -15,7 +15,6 @@<br>
>> #include "llvm/ProfileData/InstrProfReader.h"<br>
>> #include "InstrProfIndexed.h"<br>
>> #include "llvm/ADT/STLExtras.h"<br>
>> -#include "llvm/ProfileData/InstrProf.h"<br>
>> #include <cassert><br>
>><br>
>> using namespace llvm;<br>
>> @@ -126,18 +125,16 @@<br>
>> return error(instrprof_error::malformed);<br>
>><br>
>> // Read each counter and fill our internal storage with the values.<br>
>> - Counts.clear();<br>
>> - Counts.reserve(NumCounters);<br>
>> + Record.Counts.clear();<br>
>> + Record.Counts.reserve(NumCounters);<br>
>> for (uint64_t I = 0; I < NumCounters; ++I) {<br>
>> if (Line.is_at_end())<br>
>> return error(instrprof_error::truncated);<br>
>> uint64_t Count;<br>
>> if ((Line++)->getAsInteger(10, Count))<br>
>> return error(instrprof_error::malformed);<br>
>> - Counts.push_back(Count);<br>
>> + Record.Counts.push_back(Count);<br>
>> }<br>
>> - // Give the record a reference to our internal counter storage.<br>
>> - Record.Counts = Counts;<br>
>><br>
>> return success();<br>
>> }<br>
>> @@ -280,11 +277,10 @@<br>
>> Record.Hash = swap(Data->FuncHash);<br>
>> Record.Name = RawName;<br>
>> if (ShouldSwapBytes) {<br>
>> - Counts.clear();<br>
>> - Counts.reserve(RawCounts.size());<br>
>> + Record.Counts.clear();<br>
>> + Record.Counts.reserve(RawCounts.size());<br>
>> for (uint64_t Count : RawCounts)<br>
>> - Counts.push_back(swap(Count));<br>
>> - Record.Counts = Counts;<br>
>> + Record.Counts.push_back(swap(Count));<br>
>> } else<br>
>> Record.Counts = RawCounts;<br>
>><br>
><br>
>> @@ -303,6 +299,49 @@<br>
>> return IndexedInstrProf::ComputeHash(HashType, K);<br>
>> }<br>
>><br>
>> +typedef InstrProfLookupTrait::data_type data_type;<br>
>> +typedef InstrProfLookupTrait::offset_type offset_type;<br>
>> +<br>
>> +data_type InstrProfLookupTrait::ReadData(StringRef K, const unsigned<br>
>> char<br>
>> *D,<br>
><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>
>> + return data_type();<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>
>> + return data_type();<br>
>> +<br>
>> + CounterBuffer.clear();<br>
>> + for (unsigned J = 0; J < NumCounts; ++J)<br>
>> + CounterBuffer.push_back(endian::readNext<uint64_t, little,<br>
>> unaligned>(D));<br>
>> +<br>
>> + DataBuffer.push_back(InstrProfRecord(K, Hash, CounterBuffer));<br>
>> + }<br>
>> + return DataBuffer;<br>
>> +}<br>
>> +<br>
>> bool IndexedInstrProfReader::hasFormat(const MemoryBuffer &DataBuffer)<br>
>> {<br>
>> if (DataBuffer.getBufferSize() < 8)<br>
>> return false;<br>
><br>
>> @@ -342,8 +381,9 @@<br>
>> uint64_t HashOffset = endian::readNext<uint64_t, little,<br>
>> unaligned>(Cur);<br>
>><br>
>> // The rest of the file is an on disk hash table.<br>
>> - Index.reset(InstrProfReaderIndex::Create(Start + HashOffset, Cur,<br>
>> Start,<br>
>> -<br>
>> InstrProfLookupTrait(HashType)));<br>
>> + Index.reset(InstrProfReaderIndex::Create(<br>
>> + Start + HashOffset, Cur, Start,<br>
>> + InstrProfLookupTrait(HashType, FormatVersion)));<br>
>> // Set up our iterator for readNextRecord.<br>
>> RecordIterator = Index->data_begin();<br>
>><br>
>> @@ -357,21 +397,14 @@<br>
><br>
>> return error(instrprof_error::unknown_function);<br>
>><br>
>> // Found it. Look for counters with the right hash.<br>
>> - ArrayRef<uint64_t> Data = (*Iter).Data;<br>
>> - uint64_t NumCounts;<br>
>> - for (uint64_t I = 0, E = Data.size(); I != E; I += NumCounts) {<br>
>> - // The function hash comes first.<br>
>> - uint64_t FoundHash = Data[I++];<br>
>> - // In v1, we have at least one count. Later, we have the number of<br>
>> counts.<br>
>> - if (I == E)<br>
>> - return error(instrprof_error::malformed);<br>
>> - NumCounts = FormatVersion == 1 ? E - I : Data[I++];<br>
>> - // If we have more counts than data, this is bogus.<br>
>> - if (I + NumCounts > E)<br>
>> - return error(instrprof_error::malformed);<br>
>> + ArrayRef<InstrProfRecord> Data = (*Iter);<br>
>> + if (Data.empty())<br>
>> + return error(instrprof_error::malformed);<br>
>> +<br>
>> + for (unsigned I = 0, E = Data.size(); I < E; ++I) {<br>
>> // Check for a match and fill the vector if there is one.<br>
>> - if (FoundHash == FuncHash) {<br>
>> - Counts = Data.slice(I, NumCounts);<br>
>> + if (Data[I].Hash == FuncHash) {<br>
>> + Counts = Data[I].Counts;<br>
>> return success();<br>
>> }<br>
>> }<br>
><br>
>> @@ -384,30 +417,15 @@<br>
><br>
>> if (RecordIterator == Index->data_end())<br>
>> return error(instrprof_error::eof);<br>
>><br>
>> - // Record the current function name.<br>
>> - Record.Name = (*RecordIterator).Name;<br>
>> -<br>
>> - ArrayRef<uint64_t> Data = (*RecordIterator).Data;<br>
>> - // Valid data starts with a hash and either a count or the number of<br>
>> counts.<br>
>> - if (CurrentOffset + 1 > Data.size())<br>
>> - return error(instrprof_error::malformed);<br>
>> - // First we have a function hash.<br>
>> - Record.Hash = Data[CurrentOffset++];<br>
>> - // In version 1 we knew the number of counters implicitly, but in<br>
>> newer<br>
>> - // versions we store the number of counters next.<br>
>> - uint64_t NumCounts =<br>
>> - FormatVersion == 1 ? Data.size() - CurrentOffset :<br>
>> Data[CurrentOffset++];<br>
>> - if (CurrentOffset + NumCounts > Data.size())<br>
>> + if ((*RecordIterator).empty())<br>
>> return error(instrprof_error::malformed);<br>
>> - // And finally the counts themselves.<br>
>> - Record.Counts = Data.slice(CurrentOffset, NumCounts);<br>
>><br>
>> - // If we've exhausted this function's data, increment the record.<br>
>> - CurrentOffset += NumCounts;<br>
>> - if (CurrentOffset == Data.size()) {<br>
>> + static unsigned RecordIndex = 0;<br>
>> + ArrayRef<InstrProfRecord> Data = (*RecordIterator);<br>
>> + Record = Data[RecordIndex++];<br>
>> + if (RecordIndex >= Data.size()) {<br>
>> ++RecordIterator;<br>
>> - CurrentOffset = 0;<br>
>> + RecordIndex = 0;<br>
>> }<br>
>> -<br>
>> return success();<br>
>> }<br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
</div></div>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
><br>
><br>
<br>
<br>
</blockquote></div><br></div></div>