<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jun 20, 2015 at 9:27 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">><br>
><br>
> On Sat, Jun 20, 2015 at 8:04 PM, Betul Buyukkurt <<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a><br>
</span><span class="">> <mailto:<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>> > wrote:<br>
><br>
><br>
>><br>
>><br>
>> On Sat, Jun 20, 2015 at 3:15 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a><br>
>> <mailto:<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>><br>
</span><span class="">>> <mailto:<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a> <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>
</span>>> <mailto:<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">><br>
>> 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=OoitiFZrTnhzk-bCkBBX1cLYx_p-PYuwsVVpaAdPias&s=uSFCb0cl_-_8cr53oz1WXpfDcRc5wfzRbUBdk7sinsQ&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=OoitiFZrTnhzk-bCkBBX1cLYx_p-PYuwsVVpaAdPias&s=FQbgw3KAu6UMHDBKglTVp-sv1raUXSWwXIFcw1voMP4&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>
><br>
> 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>
><br>
><br>
><br>
> Looks like your current version is passing by rvalue reference:<br>
><br>
><br>
> + InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t><br>
> &&Counts)<br>
> + : Name(Name), Hash(Hash), Counts(Counts) {}<br>
><br>
> What I was suggesting was to pass by value:<br>
><br>
><br>
> + InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t><br>
> Counts)<br>
> + : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}<br>
><br>
> Works just as well, but lets callers /not/ pass ownership if they don't<br>
> want<br>
> to.<br>
<br>
</div></div>Then what I understood from your first comment was correct. I purposefully<br>
wanted to avoid this to prevent copying of data around.<br></blockquote><div><br>So long as the user passes a temporary (or a std::move of a variable, etc) there will be no copying. It's generally not important for an interface to stop a caller from producing copies - if the caller wants to produce a copy, that's up to the caller.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Oh, and actually you forgot the std::move in the ctor init list (perhaps<br>
> that's what you thought I meant by pass by value? Sorry for the<br>
> confusion/lack of clarity)<br>
<br>
</span>I'm new to the rvalue references. I do think Counts(Counts) knowing what<br>
it received is an rvalue reference would use the<br>
"vector(vector&& other)" constructor, would it not?<br></blockquote><div><br>Unfortunately not - once the value has a name it's no longer an rvalue reference... it's weird.<br><br>Try this:<br><br>std::unique_ptr<int> x;<br>std::unique_ptr<int> &&y = std::move(x);<br>std::unique_ptr<int> z = y;<br><br>that last line doesn't compile without a std::move on it.<br><br>(using unique_ptr's a great way to build up an intuition about when moves and copies happen - everywhere a copy would happen, the code will fail to compile - then you can apply that intuition to move-and-copyable types like the vector you have there)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
><br>
><br>
> -Betul<br>
><br>
><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<br>
>>> 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 &) =<br>
>>> 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<br>
>>> 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,<br>
>>> 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>
><br>
>> <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>
</div></div>>> <mailto:<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>
><br>
><br>
><br>
><br>
<br>
<br>
</blockquote></div><br></div></div>