[PATCH] Value profiling - patchset 2 - merge intended
Betul Buyukkurt
betulb at codeaurora.org
Sat Jun 20 20:04:14 PDT 2015
>
>
> 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.
-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
>
>
>
>
More information about the llvm-commits
mailing list