[PATCH] Value profiling - patchset 2 - merge intended
David Blaikie
dblaikie at gmail.com
Sat Jun 20 21:33:16 PDT 2015
On Sat, Jun 20, 2015 at 9:27 PM, Betul Buyukkurt <betulb at codeaurora.org>
wrote:
> >
> >
> > On Sat, Jun 20, 2015 at 8:04 PM, Betul Buyukkurt <betulb at codeaurora.org
> > <mailto:betulb at codeaurora.org> > wrote:
> >
> >
> >>
> >>
> >> On Sat, Jun 20, 2015 at 3:15 PM, Justin Bogner <mail at justinbogner.com
> >> <mailto:mail at justinbogner.com>
> >> <mailto:mail at justinbogner.com <mailto:mail at justinbogner.com> > > wrote:
> >>
> >>
> >> Betul Buyukkurt <betulb at codeaurora.org <mailto:betulb at codeaurora.org>
> >> <mailto:betulb at codeaurora.org <mailto:betulb at codeaurora.org> > >
> >
> >> writes:
> >>> Included <cstdint> to InstrProf.h
> >>>
> >>>
> >>> http://reviews.llvm.org/D10493
> >>>
> >>> Files:
> >>> include/llvm/ProfileData/InstrProf.h
> >>> include/llvm/ProfileData/InstrProfReader.h
> >>> lib/ProfileData/InstrProfReader.cpp
> >>>
> >>> EMAIL PREFERENCES
> >>> http://reviews.llvm.org/settings/panel/emailpreferences/
> >>
> >>> Index: include/llvm/ProfileData/InstrProf.h
> >>> ===================================================================
> >>> --- include/llvm/ProfileData/InstrProf.h
> >>> +++ include/llvm/ProfileData/InstrProf.h
> >>> @@ -16,7 +16,10 @@
> >>> #ifndef LLVM_PROFILEDATA_INSTRPROF_H_
> >>> #define LLVM_PROFILEDATA_INSTRPROF_H_
> >>>
> >>> +#include "llvm/ADT/StringRef.h"
> >>> +#include <cstdint>
> >>> #include <system_error>
> >>> +#include <vector>
> >>>
> >>> namespace llvm {
> >>> const std::error_category &instrprof_category();
> >>> @@ -41,6 +44,16 @@
> >>> return std::error_code(static_cast<int>(E), instrprof_category());
> >>> }
> >>>
> >>> +/// Profiling information for a single function.
> >>> +struct InstrProfRecord {
> >>> + InstrProfRecord() {}
> >>> + InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t>
> >>> &Counts)
> >>
> >> Please use an rvalue reference for Counts.
> >>
> >>
> >>
> >> Generally pass-by-value's the right tool in my experience. That way the
> >> caller can decide whether to move into the operand or copy, depending on
> >> their needs. The callee doesn't have to force the caller one way or
> >> another.
> >
> >
> > I totally agree. I did not understand what was meant by pass by value in
> > the previous comment. I now understood the concern. I fixed it in my
> > latest patch. Thanks for the comments.
> >
> >
> >
> > Looks like your current version is passing by rvalue reference:
> >
> >
> > + InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t>
> > &&Counts)
> > + : Name(Name), Hash(Hash), Counts(Counts) {}
> >
> > What I was suggesting was to pass by value:
> >
> >
> > + InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t>
> > Counts)
> > + : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}
> >
> > Works just as well, but lets callers /not/ pass ownership if they don't
> > want
> > to.
>
> Then what I understood from your first comment was correct. I purposefully
> wanted to avoid this to prevent copying of data around.
>
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.
>
> > 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)
>
> I'm new to the rvalue references. I do think Counts(Counts) knowing what
> it received is an rvalue reference would use the
> "vector(vector&& other)" constructor, would it not?
>
Unfortunately not - once the value has a name it's no longer an rvalue
reference... it's weird.
Try this:
std::unique_ptr<int> x;
std::unique_ptr<int> &&y = std::move(x);
std::unique_ptr<int> z = y;
that last line doesn't compile without a std::move on it.
(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)
>
> >
> >
> > -Betul
> >
> >
> >>
> >>
> >>> + : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}
> >>> + StringRef Name;
> >>> + uint64_t Hash;
> >>> + std::vector<uint64_t> Counts;
> >>> +};
> >>> +
> >>> } // end namespace llvm
> >>>
> >>> namespace std {
> >>> Index: include/llvm/ProfileData/InstrProfReader.h
> >>> ===================================================================
> >>> --- include/llvm/ProfileData/InstrProfReader.h
> >>> +++ include/llvm/ProfileData/InstrProfReader.h
> >>> @@ -29,16 +29,6 @@
> >>>
> >>> class InstrProfReader;
> >>>
> >>> -/// Profiling information for a single function.
> >>> -struct InstrProfRecord {
> >>> - InstrProfRecord() {}
> >>> - InstrProfRecord(StringRef Name, uint64_t Hash, ArrayRef<uint64_t>
> >>> Counts)
> >>> - : Name(Name), Hash(Hash), Counts(Counts) {}
> >>> - StringRef Name;
> >>> - uint64_t Hash;
> >>> - ArrayRef<uint64_t> Counts;
> >>> -};
> >>> -
> >>> /// A file format agnostic iterator over profiling data.
> >>> class InstrProfIterator : public
> >>> std::iterator<std::input_iterator_tag,
> >>> InstrProfRecord> {
> >>> @@ -114,8 +104,6 @@
> >>> std::unique_ptr<MemoryBuffer> DataBuffer;
> >>> /// Iterator over the profile data.
> >>> line_iterator Line;
> >>> - /// The current set of counter values.
> >>> - std::vector<uint64_t> Counts;
> >>>
> >>> TextInstrProfReader(const TextInstrProfReader &) = delete;
> >>> TextInstrProfReader &operator=(const TextInstrProfReader &) =
> >>> delete;
> >>> @@ -141,8 +129,6 @@
> >>> private:
> >>> /// The profile data file contents.
> >>> std::unique_ptr<MemoryBuffer> DataBuffer;
> >>> - /// The current set of counter values.
> >>> - std::vector<uint64_t> Counts;
> >>> struct ProfileData {
> >>> const uint32_t NameSize;
> >>> const uint32_t NumCounters;
> >>
> >>> @@ -206,17 +192,16 @@
> >>> /// Trait for lookups into the on-disk hash table for the binary
> >>> instrprof
> >>> /// format.
> >>> class InstrProfLookupTrait {
> >>> - std::vector<uint64_t> DataBuffer;
> >>> + std::vector<InstrProfRecord> DataBuffer;
> >>> IndexedInstrProf::HashT HashType;
> >>> + unsigned FormatVersion;
> >>> +
> >>> public:
> >>> - InstrProfLookupTrait(IndexedInstrProf::HashT HashType) :
> >>> HashType(HashType) {}
> >>> + InstrProfLookupTrait(IndexedInstrProf::HashT HashType, unsigned
> >>> FormatVersion)
> >>> + : HashType(HashType), FormatVersion(FormatVersion) {}
> >>> +
> >>> + typedef ArrayRef<InstrProfRecord> data_type;
> >>>
> >>> - struct data_type {
> >>> - data_type(StringRef Name, ArrayRef<uint64_t> Data)
> >>> - : Name(Name), Data(Data) {}
> >>> - StringRef Name;
> >>> - ArrayRef<uint64_t> Data;
> >>> - };
> >>> typedef StringRef internal_key_type;
> >>> typedef StringRef external_key_type;
> >>> typedef uint64_t hash_value_type;
> >>> @@ -239,22 +224,9 @@
> >>> return StringRef((const char *)D, N);
> >>> }
> >>>
> >>> - data_type ReadData(StringRef K, const unsigned char *D, offset_type
> >>> N)
> >>> {
> >>> - DataBuffer.clear();
> >>> - if (N % sizeof(uint64_t))
> >>> - // The data is corrupt, don't try to read it.
> >>> - return data_type("", DataBuffer);
> >>> -
> >>> - using namespace support;
> >>> - // We just treat the data as opaque here. It's simpler to handle
> >>> in
> >>> - // IndexedInstrProfReader.
> >>> - unsigned NumEntries = N / sizeof(uint64_t);
> >>> - DataBuffer.reserve(NumEntries);
> >>> - for (unsigned I = 0; I < NumEntries; ++I)
> >>> - DataBuffer.push_back(endian::readNext<uint64_t, little,
> >>> unaligned>(D));
> >>> - return data_type(K, DataBuffer);
> >>> - }
> >>> + data_type ReadData(StringRef K, const unsigned char *D, offset_type
> >>> N);
> >>> };
> >>> +
> >>> typedef OnDiskIterableChainedHashTable<InstrProfLookupTrait>
> >>> InstrProfReaderIndex;
> >>>
> >>> @@ -267,8 +239,6 @@
> >>> std::unique_ptr<InstrProfReaderIndex> Index;
> >>> /// Iterator over the profile data.
> >>> InstrProfReaderIndex::data_iterator RecordIterator;
> >>> - /// Offset into our current data set.
> >>> - size_t CurrentOffset;
> >>> /// The file format version of the profile data.
> >>> uint64_t FormatVersion;
> >>> /// The maximal execution count among all functions.
> >>> @@ -278,7 +248,7 @@
> >>
> >>> IndexedInstrProfReader &operator=(const IndexedInstrProfReader &) =
> >>> delete;
> >>> public:
> >>> IndexedInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer)
> >>> - : DataBuffer(std::move(DataBuffer)), Index(nullptr),
> >>> CurrentOffset(0) {}
> >>> + : DataBuffer(std::move(DataBuffer)), Index(nullptr) {}
> >>>
> >>> /// Return true if the given buffer is in an indexed instrprof
> >>> format.
> >>> static bool hasFormat(const MemoryBuffer &DataBuffer);
> >>> Index: lib/ProfileData/InstrProfReader.cpp
> >>> ===================================================================
> >>> --- lib/ProfileData/InstrProfReader.cpp
> >>> +++ lib/ProfileData/InstrProfReader.cpp
> >>> @@ -15,7 +15,6 @@
> >>> #include "llvm/ProfileData/InstrProfReader.h"
> >>> #include "InstrProfIndexed.h"
> >>> #include "llvm/ADT/STLExtras.h"
> >>> -#include "llvm/ProfileData/InstrProf.h"
> >>> #include <cassert>
> >>>
> >>> using namespace llvm;
> >>> @@ -126,18 +125,16 @@
> >>> return error(instrprof_error::malformed);
> >>>
> >>> // Read each counter and fill our internal storage with the values.
> >>> - Counts.clear();
> >>> - Counts.reserve(NumCounters);
> >>> + Record.Counts.clear();
> >>> + Record.Counts.reserve(NumCounters);
> >>> for (uint64_t I = 0; I < NumCounters; ++I) {
> >>> if (Line.is_at_end())
> >>> return error(instrprof_error::truncated);
> >>> uint64_t Count;
> >>> if ((Line++)->getAsInteger(10, Count))
> >>> return error(instrprof_error::malformed);
> >>> - Counts.push_back(Count);
> >>> + Record.Counts.push_back(Count);
> >>> }
> >>> - // Give the record a reference to our internal counter storage.
> >>> - Record.Counts = Counts;
> >>>
> >>> return success();
> >>> }
> >>> @@ -280,11 +277,10 @@
> >>> Record.Hash = swap(Data->FuncHash);
> >>> Record.Name = RawName;
> >>> if (ShouldSwapBytes) {
> >>> - Counts.clear();
> >>> - Counts.reserve(RawCounts.size());
> >>> + Record.Counts.clear();
> >>> + Record.Counts.reserve(RawCounts.size());
> >>> for (uint64_t Count : RawCounts)
> >>> - Counts.push_back(swap(Count));
> >>> - Record.Counts = Counts;
> >>> + Record.Counts.push_back(swap(Count));
> >>> } else
> >>> Record.Counts = RawCounts;
> >>>
> >>
> >>> @@ -303,6 +299,49 @@
> >>> return IndexedInstrProf::ComputeHash(HashType, K);
> >>> }
> >>>
> >>> +typedef InstrProfLookupTrait::data_type data_type;
> >>> +typedef InstrProfLookupTrait::offset_type offset_type;
> >>> +
> >>> +data_type InstrProfLookupTrait::ReadData(StringRef K, const unsigned
> >>> char
> >>> *D,
> >>
> >>> + offset_type N) {
> >>> +
> >>> + // Check if the data is corrupt. If so, don't try to read it.
> >>> + if (N % sizeof(uint64_t))
> >>> + return data_type();
> >>> +
> >>> + DataBuffer.clear();
> >>> + uint64_t NumCounts;
> >>> + uint64_t NumEntries = N / sizeof(uint64_t);
> >>> + std::vector<uint64_t> CounterBuffer;
> >>> + for (uint64_t I = 0; I < NumEntries; I += NumCounts) {
> >>> + using namespace support;
> >>> + // The function hash comes first.
> >>> + uint64_t Hash = endian::readNext<uint64_t, little, unaligned>(D);
> >>> +
> >>> + if (++I >= NumEntries)
> >>> + return data_type();
> >>> +
> >>> + // In v1, we have at least one count.
> >>> + // Later, we have the number of counts.
> >>> + NumCounts = (1 == FormatVersion)
> >>> + ? NumEntries - I
> >>> + : endian::readNext<uint64_t, little,
> >>> unaligned>(D);
> >>> + if (1 != FormatVersion)
> >>> + ++I;
> >>> +
> >>> + // If we have more counts than data, this is bogus.
> >>> + if (I + NumCounts > NumEntries)
> >>> + return data_type();
> >>> +
> >>> + CounterBuffer.clear();
> >>> + for (unsigned J = 0; J < NumCounts; ++J)
> >>> + CounterBuffer.push_back(endian::readNext<uint64_t, little,
> >>> unaligned>(D));
> >>> +
> >>> + DataBuffer.push_back(InstrProfRecord(K, Hash, CounterBuffer));
> >>> + }
> >>> + return DataBuffer;
> >>> +}
> >>> +
> >>> bool IndexedInstrProfReader::hasFormat(const MemoryBuffer &DataBuffer)
> >>> {
> >>> if (DataBuffer.getBufferSize() < 8)
> >>> return false;
> >>
> >>> @@ -342,8 +381,9 @@
> >>> uint64_t HashOffset = endian::readNext<uint64_t, little,
> >>> unaligned>(Cur);
> >>>
> >>> // The rest of the file is an on disk hash table.
> >>> - Index.reset(InstrProfReaderIndex::Create(Start + HashOffset, Cur,
> >>> Start,
> >>> -
> >>> InstrProfLookupTrait(HashType)));
> >>> + Index.reset(InstrProfReaderIndex::Create(
> >>> + Start + HashOffset, Cur, Start,
> >>> + InstrProfLookupTrait(HashType, FormatVersion)));
> >>> // Set up our iterator for readNextRecord.
> >>> RecordIterator = Index->data_begin();
> >>>
> >>> @@ -357,21 +397,14 @@
> >>
> >>> return error(instrprof_error::unknown_function);
> >>>
> >>> // Found it. Look for counters with the right hash.
> >>> - ArrayRef<uint64_t> Data = (*Iter).Data;
> >>> - uint64_t NumCounts;
> >>> - for (uint64_t I = 0, E = Data.size(); I != E; I += NumCounts) {
> >>> - // The function hash comes first.
> >>> - uint64_t FoundHash = Data[I++];
> >>> - // In v1, we have at least one count. Later, we have the number of
> >>> counts.
> >>> - if (I == E)
> >>> - return error(instrprof_error::malformed);
> >>> - NumCounts = FormatVersion == 1 ? E - I : Data[I++];
> >>> - // If we have more counts than data, this is bogus.
> >>> - if (I + NumCounts > E)
> >>> - return error(instrprof_error::malformed);
> >>> + ArrayRef<InstrProfRecord> Data = (*Iter);
> >>> + if (Data.empty())
> >>> + return error(instrprof_error::malformed);
> >>> +
> >>> + for (unsigned I = 0, E = Data.size(); I < E; ++I) {
> >>> // Check for a match and fill the vector if there is one.
> >>> - if (FoundHash == FuncHash) {
> >>> - Counts = Data.slice(I, NumCounts);
> >>> + if (Data[I].Hash == FuncHash) {
> >>> + Counts = Data[I].Counts;
> >>> return success();
> >>> }
> >>> }
> >>
> >>> @@ -384,30 +417,15 @@
> >>
> >>> if (RecordIterator == Index->data_end())
> >>> return error(instrprof_error::eof);
> >>>
> >>> - // Record the current function name.
> >>> - Record.Name = (*RecordIterator).Name;
> >>> -
> >>> - ArrayRef<uint64_t> Data = (*RecordIterator).Data;
> >>> - // Valid data starts with a hash and either a count or the number of
> >>> counts.
> >>> - if (CurrentOffset + 1 > Data.size())
> >>> - return error(instrprof_error::malformed);
> >>> - // First we have a function hash.
> >>> - Record.Hash = Data[CurrentOffset++];
> >>> - // In version 1 we knew the number of counters implicitly, but in
> >>> newer
> >>> - // versions we store the number of counters next.
> >>> - uint64_t NumCounts =
> >>> - FormatVersion == 1 ? Data.size() - CurrentOffset :
> >>> Data[CurrentOffset++];
> >>> - if (CurrentOffset + NumCounts > Data.size())
> >>> + if ((*RecordIterator).empty())
> >>> return error(instrprof_error::malformed);
> >>> - // And finally the counts themselves.
> >>> - Record.Counts = Data.slice(CurrentOffset, NumCounts);
> >>>
> >>> - // If we've exhausted this function's data, increment the record.
> >>> - CurrentOffset += NumCounts;
> >>> - if (CurrentOffset == Data.size()) {
> >>> + static unsigned RecordIndex = 0;
> >>> + ArrayRef<InstrProfRecord> Data = (*RecordIterator);
> >>> + Record = Data[RecordIndex++];
> >>> + if (RecordIndex >= Data.size()) {
> >>> ++RecordIterator;
> >>> - CurrentOffset = 0;
> >>> + RecordIndex = 0;
> >>> }
> >>> -
> >>> return success();
> >>> }
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >
> >> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> >> <mailto:llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu> >
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> >
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150620/496ac57f/attachment.html>
More information about the llvm-commits
mailing list