[PATCH] D10674: Value profiling - patchset 3

Betul Buyukkurt via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 28 10:47:57 PDT 2015


>
>> Betul Buyukkurt <betulb at codeaurora.org> writes:
>>> betulb updated this revision to Diff 35117.
>>> betulb added a comment.
>>> Review comments addressed:
>>> - Removed the ValuesSites[instrprof_value_kind] array and made
>>> individual value site vectors for different kinds. Currently only one
> value site vector is present due to single value kind is valid. - Added
> const std::vector<InstrProfValueSiteRecord>&
>>> getValueSitesForKind(uint32_t ValueKind) const; and
>>> std::vector<InstrProfValueSiteRecord>& getValueSitesForKind(uint32_t
> ValueKind); methods to retrieve the value site vector reference for a
> given kind.
>>> - Removed the HashKeyMap member from the indexed reader.
>> This is basically good now. There are a few really minor things left,
> then I think this can go in.
>
> It seems like the latest patch addresses all the comments except the
> const_cast comment. I'll update my patch right away.
>
> Thanks,
> -Betul
>
>>> http://reviews.llvm.org/D10674
>>> Files:
>>>   include/llvm/ProfileData/InstrProf.h
>>>   include/llvm/ProfileData/InstrProfReader.h
>>>   include/llvm/ProfileData/InstrProfWriter.h
>>>   lib/ProfileData/InstrProf.cpp
>>>   lib/ProfileData/InstrProfIndexed.h
>>>   lib/ProfileData/InstrProfReader.cpp
>>>   lib/ProfileData/InstrProfWriter.cpp
>>>   tools/llvm-profdata/llvm-profdata.cpp
>>>   unittests/ProfileData/CoverageMappingTest.cpp
>>>   unittests/ProfileData/InstrProfTest.cpp
>>> Index: unittests/ProfileData/InstrProfTest.cpp
>>> =================================================================== ---
> unittests/ProfileData/InstrProfTest.cpp
>>> +++ unittests/ProfileData/InstrProfTest.cpp
>>> @@ -50,7 +50,8 @@
>>>  }
>>>  TEST_F(InstrProfTest, write_and_read_one_function) {
>>> -  Writer.addFunctionCounts("foo", 0x1234, {1, 2, 3, 4});
>>> +  InstrProfRecord Record("foo", 0x1234, {1, 2, 3, 4});
>>> +  Writer.addRecord(std::move(Record));
>>>    auto Profile = Writer.writeBuffer();
>>>    readProfile(std::move(Profile));
>>> @@ -67,8 +68,10 @@
>>>  }
>>>  TEST_F(InstrProfTest, get_function_counts) {
>>> -  Writer.addFunctionCounts("foo", 0x1234, {1, 2});
>>> -  Writer.addFunctionCounts("foo", 0x1235, {3, 4});
>>> +  InstrProfRecord Record1("foo", 0x1234, {1, 2});
>>> +  InstrProfRecord Record2("foo", 0x1235, {3, 4});
>>> +  Writer.addRecord(std::move(Record1));
>>> +  Writer.addRecord(std::move(Record2));
>>>    auto Profile = Writer.writeBuffer();
>>>    readProfile(std::move(Profile));
>>> @@ -92,9 +95,12 @@
>>>  }
>>>  TEST_F(InstrProfTest, get_max_function_count) {
>>> -  Writer.addFunctionCounts("foo", 0x1234, {1ULL << 31, 2});
>>> -  Writer.addFunctionCounts("bar", 0, {1ULL << 63});
>>> -  Writer.addFunctionCounts("baz", 0x5678, {0, 0, 0, 0});
>>> +  InstrProfRecord Record1("foo", 0x1234, {1ULL << 31, 2});
>>> +  InstrProfRecord Record2("bar", 0, {1ULL << 63});
>>> +  InstrProfRecord Record3("baz", 0x5678, {0, 0, 0, 0});
>>> +  Writer.addRecord(std::move(Record1));
>>> +  Writer.addRecord(std::move(Record2));
>>> +  Writer.addRecord(std::move(Record3));
>>>    auto Profile = Writer.writeBuffer();
>>>    readProfile(std::move(Profile));
>>> Index: unittests/ProfileData/CoverageMappingTest.cpp
>>> =================================================================== ---
> unittests/ProfileData/CoverageMappingTest.cpp
>>> +++ unittests/ProfileData/CoverageMappingTest.cpp
>>> @@ -188,7 +188,8 @@
>>>  }
>>>  TEST_F(CoverageMappingTest, basic_coverage_iteration) {
>>> -  ProfileWriter.addFunctionCounts("func", 0x1234, {30, 20, 10, 0}); +
> InstrProfRecord Record("func", 0x1234, {30, 20, 10, 0});
>>> +  ProfileWriter.addRecord(std::move(Record));
>>>    readProfCounts();
>>>    addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
>>> @@ -238,7 +239,8 @@
>>>  }
>>>  TEST_F(CoverageMappingTest, combine_regions) {
>>> -  ProfileWriter.addFunctionCounts("func", 0x1234, {10, 20, 30}); +
> InstrProfRecord Record("func", 0x1234, {10, 20, 30});
>>> +  ProfileWriter.addRecord(std::move(Record));
>>>    readProfCounts();
>>>    addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
>>> @@ -256,7 +258,8 @@
>>>  }
>>>  TEST_F(CoverageMappingTest, dont_combine_expansions) {
>>> -  ProfileWriter.addFunctionCounts("func", 0x1234, {10, 20});
>>> +  InstrProfRecord Record("func", 0x1234, {10, 20});
>>> +  ProfileWriter.addRecord(std::move(Record));
>>>    readProfCounts();
>>>    addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
>>> @@ -275,7 +278,8 @@
>>>  }
>>>  TEST_F(CoverageMappingTest, strip_filename_prefix) {
>>> -  ProfileWriter.addFunctionCounts("file1:func", 0x1234, {10}); +
> InstrProfRecord Record("file1:func", 0x1234, {10});
>>> +  ProfileWriter.addRecord(std::move(Record));
>>>    readProfCounts();
>>>    addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
>>> Index: tools/llvm-profdata/llvm-profdata.cpp
>>> =================================================================== ---
> tools/llvm-profdata/llvm-profdata.cpp
>>> +++ tools/llvm-profdata/llvm-profdata.cpp
>>> @@ -58,9 +58,8 @@
>>>        exitWithError(ec.message(), Filename);
>>>      auto Reader = std::move(ReaderOrErr.get());
>>> -    for (const auto &I : *Reader)
>>> -      if (std::error_code EC =
>>> -              Writer.addFunctionCounts(I.Name, I.Hash, I.Counts)) +
> for (auto &I : *Reader)
>>> +      if (std::error_code EC = Writer.addRecord(std::move(I)))
>>>          errs() << Filename << ": " << I.Name << ": " << EC.message()
> <<
>> "\n";
>>>      if (Reader->hasError())
>>>        exitWithError(Reader->getError().message(), Filename);
>>> @@ -134,8 +133,8 @@
>>>  }
>>>  static int showInstrProfile(std::string Filename, bool ShowCounts,
>>> -                            bool ShowAllFunctions, std::string
>> ShowFunction,
>>> -                            raw_fd_ostream &OS) {
>>> +                            bool ShowIndirectCallTargets, bool
>> ShowAllFunctions,
>>> +                            std::string ShowFunction, raw_fd_ostream
>> &OS) {
>>>    auto ReaderOrErr = InstrProfReader::create(Filename);
>>>    if (std::error_code EC = ReaderOrErr.getError())
>>>      exitWithError(EC.message(), Filename);
>>> @@ -162,6 +161,10 @@
>>>           << "    Hash: " << format("0x%016" PRIx64, Func.Hash) << "\n"
> << "    Counters: " << Func.Counts.size() << "\n"
>>>           << "    Function count: " << Func.Counts[0] << "\n";
>>> +      if (ShowIndirectCallTargets)
>>> +        OS << "    Indirect Call Site Count: "
>>> +           << Func.IndirectCallSites.size()
>>> +           << "\n";
>>>      }
>>>      if (Show && ShowCounts)
>>> @@ -174,6 +177,16 @@
>>>      }
>>>      if (Show && ShowCounts)
>>>        OS << "]\n";
>>> +
>>> +    if (Show && ShowIndirectCallTargets) {
>>> +      OS << "    Indirect Target Results: \n";
>>> +      for (size_t I = 0, E = Func.IndirectCallSites.size(); I < E;
> ++I)
>> {
>>> +        for (auto V : Func.IndirectCallSites[I].ValueData) {
>>> +          OS << "\t[ " << I << ", ";
>>> +          OS << (const char *)V.first << ", " << V.second << " ]\n"; +
>        }
>>> +      }
>>> +    }
>>>    }
>>>    if (Reader->hasError())
>>>      exitWithError(Reader->getError().message(), Filename);
>>> @@ -212,6 +225,8 @@
>>>    cl::opt<bool> ShowCounts("counts", cl::init(false),
>>>                             cl::desc("Show counter values for shown
>> functions"));
>>> +  cl::opt<bool> ShowIndirectCallTargets("ic-targets", cl::init(false),
> +      cl::desc("Show indirect call site target values for shown
>> functions"));
>>>    cl::opt<bool> ShowAllFunctions("all-functions", cl::init(false),
>>>                                   cl::desc("Details for every
>> function"));
>>>    cl::opt<std::string> ShowFunction("function",
>>> @@ -240,8 +255,8 @@
>>>      errs() << "warning: -function argument ignored: showing all
>> functions\n";
>>>    if (ProfileKind == instr)
>>> -    return showInstrProfile(Filename, ShowCounts, ShowAllFunctions, -
>                           ShowFunction, OS);
>>> +    return showInstrProfile(Filename, ShowCounts,
>> ShowIndirectCallTargets,
>>> +                            ShowAllFunctions, ShowFunction, OS);
>>>    else
>>>      return showSampleProfile(Filename, ShowCounts, ShowAllFunctions,
>>>                               ShowFunction, OS);
>>> Index: lib/ProfileData/InstrProfWriter.cpp
>>> =================================================================== ---
> lib/ProfileData/InstrProfWriter.cpp
>>> +++ lib/ProfileData/InstrProfWriter.cpp
>>> @@ -26,8 +26,8 @@
>>>    typedef StringRef key_type;
>>>    typedef StringRef key_type_ref;
>>> -  typedef const InstrProfWriter::CounterData *const data_type; -
> typedef const InstrProfWriter::CounterData *const data_type_ref; +
> typedef const InstrProfWriter::ProfilingData *const data_type; +
> typedef const InstrProfWriter::ProfilingData *const data_type_ref;
>>>    typedef uint64_t hash_value_type;
>>>    typedef uint64_t offset_type;
>>> @@ -45,8 +45,27 @@
>>>      LE.write<offset_type>(N);
>>>      offset_type M = 0;
>>> -    for (const auto &Counts : *V)
>>> -      M += (2 + Counts.second.size()) * sizeof(uint64_t);
>>> +    for (const auto &ProfileData : *V) {
>>> +      M += sizeof(uint64_t); // The function hash
>>> +      M += sizeof(uint64_t); // The size of the Counts vector
>>> +      M += ProfileData.second.Counts.size() * sizeof(uint64_t); +
>>> +      // Value data
>>> +      M += sizeof(uint64_t); // Number of value kinds with value
> sites.
>>> +      for (uint32_t Kind = instrprof_value_kind::first;
>>> +           Kind < instrprof_value_kind::size; ++Kind) {
>>> +        const std::vector<InstrProfValueSiteRecord> &ValueSites = +
>         ProfileData.second.getValueSitesForKind(Kind);
>>> +        if (ValueSites.empty())
>>> +          continue;
>>> +        M += sizeof(uint64_t); // Value kind
>>> +        M += sizeof(uint64_t); // The number of value sites for given
>> value kind
>>> +        for (InstrProfValueSiteRecord I : ValueSites) {
>>> +          M += sizeof(uint64_t); // Number of value data pairs at a
>> value site
>>> +          M += 2 * sizeof(uint64_t) * I.ValueData.size(); // Value
> data
>> pairs
>>> +        }
>>> +      }
>>> +    }
>>>      LE.write<offset_type>(M);
>>>      return std::make_pair(N, M);
>>> @@ -60,52 +79,119 @@
>>>                         offset_type) {
>>>      using namespace llvm::support;
>>>      endian::Writer<little> LE(Out);
>>> -
>>> -    for (const auto &Counts : *V) {
>>> -      LE.write<uint64_t>(Counts.first);
>>> -      LE.write<uint64_t>(Counts.second.size());
>>> -      for (uint64_t I : Counts.second)
>>> +    for (const auto &ProfileData : *V) {
>>> +      LE.write<uint64_t>(ProfileData.first); // Function hash
>>> +      LE.write<uint64_t>(ProfileData.second.Counts.size());
>>> +      for (uint64_t I : ProfileData.second.Counts)
>>>          LE.write<uint64_t>(I);
>>> +
>>> +      // Compute the number of value kinds with value sites.
>>> +      uint64_t NumValueKinds = 0;
>>> +      for (uint32_t Kind = instrprof_value_kind::first;
>>> +           Kind < instrprof_value_kind::size; ++Kind)
>>> +        NumValueKinds +=
>>> +            !(ProfileData.second.getValueSitesForKind(Kind).empty());
> +      LE.write<uint64_t>(NumValueKinds);
>>> +
>>> +      // Write value data
>>> +      for (uint32_t Kind = instrprof_value_kind::first;
>>> +           Kind < instrprof_value_kind::size; ++Kind) {
>>> +        const std::vector<InstrProfValueSiteRecord> &ValueSites = +
>         ProfileData.second.getValueSitesForKind(Kind);
>>> +        if (ValueSites.empty())
>>> +          continue;
>>> +        LE.write<uint64_t>(Kind); // Write value kind
>>> +        // Write number of value sites for current value kind
>>> +        LE.write<uint64_t>(ValueSites.size());
>>> +        for (InstrProfValueSiteRecord I : ValueSites) {
>>> +          // Write number of value data pairs at this value site +
>      LE.write<uint64_t>(I.ValueData.size());
>>> +          for (auto V : I.ValueData) {
>>> +            if (Kind == instrprof_value_kind::indirect_call_target) +
>             LE.write<uint64_t>(ComputeHash((const char *)V.first)); +
>          else
>>> +              LE.write<uint64_t>(V.first);
>> We should switch over the kinds so that we get uncovered switch warnings
> here. We'll need to replace the "size" value with a "last" value so that
> this works and we don't need a case for "size", of course.
>
> Done.
>
>>> +            LE.write<uint64_t>(V.second);
>>> +          }
>>> +        }
>>> +      }
>>>      }
>>>    }
>>>  };
>>>  }
>>> -std::error_code
>>> -InstrProfWriter::addFunctionCounts(StringRef FunctionName,
>>> -                                   uint64_t FunctionHash,
>>> -                                   ArrayRef<uint64_t> Counters) { -
> auto &CounterData = FunctionData[FunctionName];
>>> +static std::error_code combineInstrProfRecords(InstrProfRecord &Dest,
> +                                               InstrProfRecord
> &Source,
>>> +                                               uint64_t
>> &MaxFunctionCount) {
>>> +  // If the number of counters doesn't match we either have bad data +
>  // or a hash collision.
>>> +  if (Dest.Counts.size() != Source.Counts.size())
>>> +    return instrprof_error::count_mismatch;
>>> -  auto Where = CounterData.find(FunctionHash);
>>> -  if (Where == CounterData.end()) {
>>> -    // We've never seen a function with this name and hash, add it. -
>   CounterData[FunctionHash] = Counters;
>>> -    // We keep track of the max function count as we go for
> simplicity.
>>> -    if (Counters[0] > MaxFunctionCount)
>>> -      MaxFunctionCount = Counters[0];
>>> -    return instrprof_error::success;
>>> +  for (size_t I = 0, E = Source.Counts.size(); I < E; ++I) {
>>> +    if (Dest.Counts[I] + Source.Counts[I] < Dest.Counts[I])
>>> +      return instrprof_error::counter_overflow;
>>> +    Dest.Counts[I] += Source.Counts[I];
>>>    }
>>> -  // We're updating a function we've seen before.
>>> -  auto &FoundCounters = Where->second;
>>> -  // If the number of counters doesn't match we either have bad data
> or
>> a hash
>>> -  // collision.
>>> -  if (FoundCounters.size() != Counters.size())
>>> -    return instrprof_error::count_mismatch;
>>> +  for (uint32_t Kind = instrprof_value_kind::first;
>>> +       Kind < instrprof_value_kind::size; ++Kind) {
>>> -  for (size_t I = 0, E = Counters.size(); I < E; ++I) {
>>> -    if (FoundCounters[I] + Counters[I] < FoundCounters[I])
>>> -      return instrprof_error::counter_overflow;
>>> -    FoundCounters[I] += Counters[I];
>>> +    std::vector<InstrProfValueSiteRecord> &SourceValueSites =
>>> +        Source.getValueSitesForKind(Kind);
>>> +    if (SourceValueSites.empty())
>>> +      continue;
>>> +
>>> +    std::vector<InstrProfValueSiteRecord> &DestValueSites =
>>> +        Dest.getValueSitesForKind(Kind);
>>> +
>>> +    if (DestValueSites.empty()) {
>>> +      DestValueSites.swap(SourceValueSites);
>>> +      continue;
>>> +    }
>>> +
>>> +    if (DestValueSites.size() != SourceValueSites.size())
>>> +      return instrprof_error::value_site_count_mismatch;
>>> +    for (size_t I = 0, E = SourceValueSites.size(); I < E; ++I) +
> DestValueSites[I].mergeValueData(SourceValueSites[I]);
>>>    }
>>> +
>>>    // We keep track of the max function count as we go for simplicity.
>>> -  if (FoundCounters[0] > MaxFunctionCount)
>>> -    MaxFunctionCount = FoundCounters[0];
>>> +  if (Dest.Counts[0] > MaxFunctionCount)
>>> +    MaxFunctionCount = Dest.Counts[0];
>>>    return instrprof_error::success;
>>>  }
>>> +void InstrProfWriter::updateStringTableReferences(InstrProfRecord &I)
> {
>>> +  I.Name = StringTable.insertString(I.Name);
>>> +  std::vector<InstrProfValueSiteRecord> &IndirectCallSites =
>>> +
>> I.getValueSitesForKind(instrprof_value_kind::indirect_call_target); This
> is just I.IndirectCallSites now.
>
> Done.
>
>>> +  for (auto& VSite : IndirectCallSites)
>>> +    for (auto& VData : VSite.ValueData)
>>> +      VData.first =
>>> +          (uint64_t)StringTable.insertString((const char
>> *)VData.first);
>> I still find it pretty questionable how we're casting back and forth
> between uint64_t and char * here. I guess I can live with it though.
>>> +}
>>> +
>>> +std::error_code InstrProfWriter::addRecord(InstrProfRecord &&I) { +
> updateStringTableReferences(I);
>>> +  auto &ProfileDataMap = FunctionData[I.Name];
>>> +
>>> +  auto Where = ProfileDataMap.find(I.Hash);
>>> +  if (Where == ProfileDataMap.end()) {
>>> +    // We've never seen a function with this name and hash, add it. +
>   ProfileDataMap[I.Hash] = I;
>>> +
>>> +    // We keep track of the max function count as we go for
> simplicity.
>>> +    if (I.Counts[0] > MaxFunctionCount)
>>> +      MaxFunctionCount = I.Counts[0];
>>> +    return instrprof_error::success;
>>> +  }
>>> +
>>> +  // We're updating a function we've seen before.
>>> +  return combineInstrProfRecords(Where->second, I, MaxFunctionCount);
> +}
>>> +
>>>  std::pair<uint64_t, uint64_t> InstrProfWriter::writeImpl(raw_ostream
>> &OS) {
>>>    OnDiskChainedHashTableGenerator<InstrProfRecordTrait> Generator;
>>> Index: lib/ProfileData/InstrProfReader.cpp
>>> =================================================================== ---
> lib/ProfileData/InstrProfReader.cpp
>>> +++ lib/ProfileData/InstrProfReader.cpp
>>> @@ -15,8 +15,12 @@
>>>  #include "llvm/ProfileData/InstrProfReader.h"
>>>  #include "InstrProfIndexed.h"
>>>  #include "llvm/ADT/STLExtras.h"
>>> +#include "llvm/Support/Debug.h"
>>> +#include "llvm/Support/raw_ostream.h"
>>>  #include <cassert>
>>> +#define DEBUG_TYPE "InstrProfReader"
>>> +
>>>  using namespace llvm;
>>>  static ErrorOr<std::unique_ptr<MemoryBuffer>>
>>> @@ -302,42 +306,96 @@
>>>  typedef InstrProfLookupTrait::data_type data_type;
>>>  typedef InstrProfLookupTrait::offset_type offset_type;
>>> +bool InstrProfLookupTrait::ReadValueProfilingData(
>>> +    const unsigned char *&D, const unsigned char *const End) { +
>>> +  using namespace support;
>>> +  // Read number of value kinds with value sites.
>>> +  if (D + sizeof(uint64_t) > End)
>>> +    return false;
>>> +  uint64_t ValueKindCount = endian::readNext<uint64_t, little,
>> unaligned>(D);
>>> +
>>> +  for (uint32_t Kind = 0; Kind < ValueKindCount; ++Kind) {
>>> +
>>> +    // Read value kind and number of value sites for kind.
>>> +    if (D + 2*sizeof(uint64_t) > End)
>>> +      return false;
>>> +    uint64_t ValueKind = endian::readNext<uint64_t, little,
>> unaligned>(D);
>>> +    uint64_t ValueSiteCount = endian::readNext<uint64_t, little,
>> unaligned>(D);
>>> +
>>> +    std::vector<InstrProfValueSiteRecord> &ValueSites =
>>> +        DataBuffer.back().getValueSitesForKind(ValueKind);
>>> +    ValueSites.reserve(ValueSiteCount);
>>> +    for (uint64_t VSite = 0; VSite < ValueSiteCount; ++VSite) { +
> // Read number of value data pairs at value site.
>>> +      if (D + sizeof(uint64_t) > End)
>>> +        return false;
>>> +      uint64_t ValueDataCount =
>>> +          endian::readNext<uint64_t, little, unaligned>(D);
>>> +
>>> +      // Check if there are as many ValueDataPairs as ValueDataCount
> in
>> memory.
>>> +      if (D + (ValueDataCount<<1)*sizeof(uint64_t) > End)
>> There's still some funny formatting. Please run clang-format-patch or
> git-clang-format on your changes.
>
> Done.
>>> +        return false;
>>> +
>>> +      InstrProfValueSiteRecord VSiteRecord;
>>> +      for (uint64_t VCount = 0; VCount < ValueDataCount; ++VCount) { +
>        uint64_t Value = endian::readNext<uint64_t, little,
>> unaligned>(D);
>>> +        uint64_t NumTaken = endian::readNext<uint64_t, little,
>> unaligned>(D);
>>> +        if (ValueKind == instrprof_value_kind::indirect_call_target) {
> +          auto Result = HashKeyMap.find(Value);
>>> +          assert(Result != HashKeyMap.end() &&
>>> +                 "Hash does not match any known keys\n");
>>> +          Value = (uint64_t)Result->second;
>>> +        }
>>> +        VSiteRecord.ValueData.push_back(std::make_pair(Value,
>> NumTaken));
>>> +      }
>>> +      ValueSites.push_back(std::move(VSiteRecord));
>>> +    }
>>> +  }
>>> +  return true;
>>> +}
>>> +
>>>  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)
>>> +  using namespace support;
>>> +  const unsigned char *End = D + N;
>>> +  while (D < End) {
>>> +    // Read hash
>>> +    if (D + sizeof(uint64_t) >= End)
>>>        return data_type();
>>> +    uint64_t Hash = endian::readNext<uint64_t, little, unaligned>(D);
> -    // 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)
>>> +    // Initialize number of counters for FormatVersion == 1
>>> +    uint64_t CountsSize = N / sizeof(uint64_t) - 1;
>>> +    // If format version is different then read number of counters +
>  if (FormatVersion != 1) {
>>> +      if (D + sizeof(uint64_t) > End)
>>> +        return data_type();
>>> +      CountsSize = endian::readNext<uint64_t, little, unaligned>(D); +
>    }
>>> +    // Read counter values
>>> +    if (D + CountsSize * sizeof(uint64_t) > End)
>>>        return data_type();
>>>      CounterBuffer.clear();
>>> -    for (unsigned J = 0; J < NumCounts; ++J)
>>> +    CounterBuffer.reserve(CountsSize);
>>> +    for (uint64_t J = 0; J < CountsSize; ++J)
>>>        CounterBuffer.push_back(endian::readNext<uint64_t, little,
>> unaligned>(D));
>>>      DataBuffer.push_back(InstrProfRecord(K, Hash,
>> std::move(CounterBuffer)));
>>> +
>>> +    // Read value profiling data
>>> +    if (FormatVersion > 2 && !ReadValueProfilingData(D, End)) { +
> DataBuffer.clear();
>>> +      return data_type();
>>> +    }
>>>    }
>>>    return DataBuffer;
>>>  }
>>> @@ -384,6 +442,23 @@
>>>    Index.reset(InstrProfReaderIndex::Create(
>>>        Start + HashOffset, Cur, Start,
>>>        InstrProfLookupTrait(HashType, FormatVersion)));
>>> +
>>> +  // Form the map of hash values to const char* keys in profiling
> data.
>>> +  std::map<uint64_t, const char *> HashKeyMap;
>>> +  for (auto Key : Index->keys()) {
>>> +    const char *KeyTableRef = StringTable.insertString(Key);
>>> +    auto Result =
>> HashKeyMap.insert(std::make_pair(ComputeHash(HashType, Key),
>>> +                                                   KeyTableRef)); +
> // Emit warning if a hash collision is detected.
>>> +    if (Result.second == false)
>>> +      DEBUG(dbgs() << "IndexedInstrProfReader: hash collision
> detected:
>> \n"
>>> +                   << "\t Map Entry(Hash, Key): " <<
>> Result.first->first
>>> +                   << ", " << Result.first->second << "\n"
>>> +                   << "\t New Entry(Hash, Key): " <<
>> ComputeHash(HashType, Key)
>>> +                   << ", " << Key << "\n");
>>> +  }
>>> +  // Set the hash key map for the InstrLookupTrait
>>> +  Index->getInfoObj().setHashKeyMap(std::move(HashKeyMap));
>> I still think a std::vector is a more appropriate type than a std::map
> here. You can just std::sort() it before passing it to setHashKeyMap and
> use std::lower_bound for O(log(N)) look ups later. This is minor though.
>
> std::map is necessary so that we may store a pair of values i.e. the hash
> and the pointer to entry for the key in the StringTable. The StringTable
> has a null terminated version of the key in the indexed file format.
>
>>>    // Set up our iterator for readNextRecord.
>>>    RecordIterator = Index->data_begin();
>>> Index: lib/ProfileData/InstrProfIndexed.h
>>> =================================================================== ---
> lib/ProfileData/InstrProfIndexed.h
>>> +++ lib/ProfileData/InstrProfIndexed.h
>>> @@ -47,7 +47,7 @@
>>>  }
>>>  const uint64_t Magic = 0x8169666f72706cff; // "\xfflprofi\x81"
>>> -const uint64_t Version = 2;
>>> +const uint64_t Version = 3;
>>>  const HashT HashType = HashT::MD5;
>>>  }
>>> Index: lib/ProfileData/InstrProf.cpp
>>> =================================================================== ---
> lib/ProfileData/InstrProf.cpp
>>> +++ lib/ProfileData/InstrProf.cpp
>>> @@ -50,6 +50,8 @@
>>>        return "Function count mismatch";
>>>      case instrprof_error::counter_overflow:
>>>        return "Counter overflow";
>>> +    case instrprof_error::value_site_count_mismatch:
>>> +      return "Function's value site counts mismatch";
>>>      }
>>>      llvm_unreachable("A value of instrprof_error has no message.");
>>>    }
>>> Index: include/llvm/ProfileData/InstrProfWriter.h
>>> =================================================================== ---
> include/llvm/ProfileData/InstrProfWriter.h
>>> +++ include/llvm/ProfileData/InstrProfWriter.h
>>> @@ -15,33 +15,32 @@
>>>  #ifndef LLVM_PROFILEDATA_INSTRPROFWRITER_H
>>>  #define LLVM_PROFILEDATA_INSTRPROFWRITER_H
>>> -#include "llvm/ADT/ArrayRef.h"
>>>  #include "llvm/ADT/DenseMap.h"
>>> -#include "llvm/ADT/StringMap.h"
>>>  #include "llvm/ProfileData/InstrProf.h"
>>>  #include "llvm/Support/DataTypes.h"
>>>  #include "llvm/Support/MemoryBuffer.h"
>>>  #include "llvm/Support/raw_ostream.h"
>>> -#include <vector>
>>>  namespace llvm {
>>>  /// Writer for instrumentation based profile data.
>>>  class InstrProfWriter {
>>>  public:
>>> -  typedef SmallDenseMap<uint64_t, std::vector<uint64_t>, 1>
>> CounterData;
>>> +  typedef SmallDenseMap<uint64_t, InstrProfRecord, 1> ProfilingData; +
>>>  private:
>>> -  StringMap<CounterData> FunctionData;
>>> +  InstrProfStringTable StringTable;
>>> +  StringMap<ProfilingData> FunctionData;
>>>    uint64_t MaxFunctionCount;
>>>  public:
>>>    InstrProfWriter() : MaxFunctionCount(0) {}
>>> +  /// Update string entries in profile data with references to
>> StringTable.
>>> +  void updateStringTableReferences(InstrProfRecord &I);
>>>    /// Add function counts for the given function. If there are already
>> counts
>>>    /// for this function and the hash and number of counts match, each
>> counter is
>>>    /// summed.
>>> -  std::error_code addFunctionCounts(StringRef FunctionName,
>>> -                                    uint64_t FunctionHash,
>>> -                                    ArrayRef<uint64_t> Counters); +
> std::error_code addRecord(InstrProfRecord &&I);
>>>    /// Write the profile to \c OS
>>>    void write(raw_fd_ostream &OS);
>>>    /// Write the profile, returning the raw data. For testing.
>>> Index: include/llvm/ProfileData/InstrProfReader.h
>>> =================================================================== ---
> include/llvm/ProfileData/InstrProfReader.h
>>> +++ include/llvm/ProfileData/InstrProfReader.h
>>> @@ -24,6 +24,7 @@
>>>  #include "llvm/Support/MemoryBuffer.h"
>>>  #include "llvm/Support/OnDiskHashTable.h"
>>>  #include <iterator>
>>> +#include <map>
>>>  namespace llvm {
>>> @@ -65,6 +66,9 @@
>>>    InstrProfIterator end() { return InstrProfIterator(); }
>>>  protected:
>>> +  /// String table for holding a unique copy of all the strings in the
>> profile.
>>> +  InstrProfStringTable StringTable;
>>> +
>>>    /// Set the current std::error_code and return same.
>>>    std::error_code error(std::error_code EC) {
>>>      LastError = EC;
>>> @@ -195,6 +199,7 @@
>>>    std::vector<InstrProfRecord> DataBuffer;
>>>    IndexedInstrProf::HashT HashType;
>>>    unsigned FormatVersion;
>>> +  std::map<uint64_t, const char *> HashKeyMap;
>>>  public:
>>>    InstrProfLookupTrait(IndexedInstrProf::HashT HashType, unsigned
>> FormatVersion)
>>> @@ -209,9 +214,13 @@
>>>    static bool EqualKey(StringRef A, StringRef B) { return A == B; }
> static StringRef GetInternalKey(StringRef K) { return K; }
>>> +  static StringRef GetExternalKey(StringRef K) { return K; }
>>>    hash_value_type ComputeHash(StringRef K);
>>> +  void setHashKeyMap(std::map<uint64_t, const char *> HashKeyMap) { +
>   this->HashKeyMap = std::move(HashKeyMap);
>>> +  }
>>>    static std::pair<offset_type, offset_type>
>>>    ReadKeyDataLength(const unsigned char *&D) {
>>>      using namespace support;
>>> @@ -224,6 +233,8 @@
>>>      return StringRef((const char *)D, N);
>>>    }
>>> +  bool ReadValueProfilingData(const unsigned char *&D,
>>> +                              const unsigned char *const End);
>>>    data_type ReadData(StringRef K, const unsigned char *D, offset_type
>> N);
>>>  };
>>> Index: include/llvm/ProfileData/InstrProf.h
>>> =================================================================== ---
> include/llvm/ProfileData/InstrProf.h
>>> +++ include/llvm/ProfileData/InstrProf.h
>>> @@ -16,42 +16,119 @@
>>>  #ifndef LLVM_PROFILEDATA_INSTRPROF_H_
>>>  #define LLVM_PROFILEDATA_INSTRPROF_H_
>>> -#include "llvm/ADT/StringRef.h"
>>> +#include "llvm/ADT/StringSet.h"
>>> +#include "llvm/Support/ErrorHandling.h"
>>>  #include <cstdint>
>>> +#include <list>
>>>  #include <system_error>
>>>  #include <vector>
>>>  namespace llvm {
>>>  const std::error_category &instrprof_category();
>>>  enum class instrprof_error {
>>> -    success = 0,
>>> -    eof,
>>> -    bad_magic,
>>> -    bad_header,
>>> -    unsupported_version,
>>> -    unsupported_hash_type,
>>> -    too_large,
>>> -    truncated,
>>> -    malformed,
>>> -    unknown_function,
>>> -    hash_mismatch,
>>> -    count_mismatch,
>>> -    counter_overflow
>>> +  success = 0,
>>> +  eof,
>>> +  bad_magic,
>>> +  bad_header,
>>> +  unsupported_version,
>>> +  unsupported_hash_type,
>>> +  too_large,
>>> +  truncated,
>>> +  malformed,
>>> +  unknown_function,
>>> +  hash_mismatch,
>>> +  count_mismatch,
>>> +  counter_overflow,
>>> +  value_site_count_mismatch
>>>  };
>>>  inline std::error_code make_error_code(instrprof_error E) {
>>>    return std::error_code(static_cast<int>(E), instrprof_category());
>>>  }
>>> +enum instrprof_value_kind : uint32_t {
>>> +  first = 0,
>>> +  indirect_call_target = 0,
>>> +  size = 1
>> The enum value names are hoisted into the enclosing scope, so names like
> first and size are going cause problems. These names need to be unique.
> We should probably use CamelCase names too, to match llvm style. ie:
>>   enum InstrProfValueKind : uint32_t {
>>     IPVK_IndirectCallTarget = 0,
>>     IPVK_First = IPVK_IndirectCallTarget,
>>     IPVK_Last = IPVK_IndirectCallTarget
>>   }
>
> Done.
>
>>> +};
>>> +
>>> +struct InstrProfStringTable {
>>> +  // Set of string values in profiling data.
>>> +  StringSet<> StringValueSet;
>>> +  InstrProfStringTable() { StringValueSet.clear(); }
>>> +  // Get a pointer to internal storage of a string in set
>>> +  const char *getStringData(StringRef Str) {
>>> +    auto Result = StringValueSet.find(Str);
>>> +    return (Result == StringValueSet.end()) ? nullptr :
>> Result->first().data();
>>> +  }
>>> +  // Insert a string to StringTable
>>> +  const char *insertString(StringRef Str) {
>>> +    auto Result = StringValueSet.insert(Str);
>>> +    return Result.first->first().data();
>>> +  }
>>> +};
>>> +
>>> +struct InstrProfValueSiteRecord {
>>> +  // Typedef for a single TargetValue-NumTaken pair.
>>> +  typedef std::pair<uint64_t, uint64_t> ValueDataPair;
>>> +  // Value profiling data pairs at a given value site.
>>> +  std::list<ValueDataPair> ValueData;
>>> +
>>> +  InstrProfValueSiteRecord() { ValueData.clear(); }
>>> +
>>> +  // Sort ValueData ascending by TargetValue
>>> +  void sortByTargetValues() {
>>> +    ValueData.sort([](const ValueDataPair &left, const ValueDataPair
>> &right) {
>>> +      return left.first < right.first;
>>> +    });
>>> +  }
>>> +  // Merge data from another InstrProfValueSiteRecord
>>> +  void mergeValueData(InstrProfValueSiteRecord &Input) {
>>> +    this->sortByTargetValues();
>>> +    Input.sortByTargetValues();
>>> +    auto I = ValueData.begin();
>>> +    auto IE = ValueData.end();
>>> +    for (auto J = Input.ValueData.begin(), JE = Input.ValueData.end();
>> J != JE;
>>> +         ++J) {
>>> +      while (I != IE && I->first < J->first)
>>> +        ++I;
>>> +      if (I != IE && I->first == J->first) {
>>> +        I->second += J->second;
>>> +        ++I;
>>> +        continue;
>>> +      }
>>> +      ValueData.insert(I, *J);
>>> +    }
>>> +  }
>>> +};
>>> +
>>>  /// Profiling information for a single function.
>>>  struct InstrProfRecord {
>>>    InstrProfRecord() {}
>>>    InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t>
>> Counts)
>>>        : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}
>>>    StringRef Name;
>>>    uint64_t Hash;
>>>    std::vector<uint64_t> Counts;
>>> +  std::vector<InstrProfValueSiteRecord> IndirectCallSites;
>>> +
>>> +  const std::vector<InstrProfValueSiteRecord>& getValueSitesForKind( +
>      uint32_t ValueKind) const {
>>> +    switch(ValueKind) {
>>> +      case instrprof_value_kind::indirect_call_target:
>>> +        return IndirectCallSites;
>>> +      default: llvm_unreachable("Unknown value kind!");
>> Get rid of the "default:" so we get covered switch warnings, and put the
> unreachable after the switch statement.
>
> Done.
>
>>> +    }
>>> +  }
>>> +
>>> +  std::vector<InstrProfValueSiteRecord>& getValueSitesForKind( +
> uint32_t ValueKind) {
>>> +    const InstrProfRecord* const_this =
>>> +        static_cast<const InstrProfRecord*>(this);
>> const_cast is probably more appropriate than static.
>
> This seems to be the only comment that I missed in my latest patch. I'll
> submit an updated patch right away.

Done. I chose to remove the static_cast<>, since it's not necessary here.
Please comment if you have additional concerns.

Thanks,
-Betul

>
>>> +    return const_cast<std::vector<InstrProfValueSiteRecord>&>
>>> +        (const_this->getValueSitesForKind(ValueKind));
>>> +  }
>>>  };
>>>  } // end namespace llvm
>
>
>
>
>




More information about the llvm-commits mailing list