[PATCH] Value profiling - patchset 2 - merge intended
David Blaikie
dblaikie at gmail.com
Sat Jun 20 20:20:21 PDT 2015
On Sat, Jun 20, 2015 at 8:04 PM, Betul Buyukkurt <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> > wrote:
> >
> >
> > Betul Buyukkurt <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.
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)
>
> -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>
> > 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/d7fe1f49/attachment.html>
More information about the llvm-commits
mailing list