[PATCH] D10674: Value profiling - patchset 3
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 28 13:17:08 PDT 2015
On Fri, Aug 28, 2015 at 1:06 PM, Betul Buyukkurt <betulb at codeaurora.org> wrote:
>>>>>> DataBuffer.push_back(InstrProfRecord(K, Hash,
>>>>> std::move(CounterBuffer)));
>>>>>> +
>>>>>> + // Read value profiling data
>>>>>> + for (uint32_t Kind = instrprof_value_kind::first;
>>>>>> + Kind < instrprof_value_kind::size; ++Kind) {
>>>>
>>>>> I still think we should store the kind in the data here and then read
>>>> the following data based on that. There's no guarantee that all kinds
>>>> that
>>>> aren't indirect_call_target will behave as they do in this loop as is,
>>>> so
>>>> I'd rather just reject outright kinds we don't understand. If the type
>>>> is
>>>> in the data we can do that, and it also saves a few bytes for functions
>>>> that don't have any value profiling data (and more bytes when we
>> support
>>>> more kinds of value profiling).
>>>>
>>>> I do not think this is possible. The format indicates that multiple
>> hash
>>>> values and data may be stored per name entry. Given that there is no
>>>> indicator in the format that tells whether the next bytes after counter
>>>> values would belong to a value kind or a hash value for the next data
>>>> for
>>>> the same key.
>>>>
>>>
>>> Sorry I missed this one. This seems to be the last main thing to be
>>> addressed/resolved/agreed upon for the indexed format.
>>>
>>> I think Justin's suggestion is possible. We just need one mandatory
>>> field to be emitted before the value profile data.
>>> value_profile_section:
>>> .quad NUM_VALUE_KIND
>>>
>>> This field indicates how many value profile kinds have profile data.
>>> When value profile data are missing, this format has storage
>>> advantage. For instance, when the function has no value profile data,
>>> The value profile section is just
>>>
>>> value_profile_section:
>>> .quad 0
>>>
>>> With Betuls patch, it will be (assuming 2 kinds)
>>>
>>> value_profile_section:
>>> .quad 0 // num of sites for kind 1 is 0
>>> .quad 0 // num of sites for kind 2 is 0
>>>
>>> However if value profile data exists for all value kinds, Betul's
>>> suggested format is slightly better -- it does not need to store the
>>> mandatory NUM_VALUE_KIND field, nor the value kind field it self.
>>>
>>> Since I believe that value profile data will be sparse (especially
>>> when we add more kinds in the future), my vote is on Justin's format
>>> which is more flexible.
>>
>> I'll make sure to address this item as well. I should respond in a day or
>> so.
>
> I've a patch that outputs the value kind into the indexed profile file
> along with the value data. However, I'd like to clarify one thing. Value
> kind should not be emitted when there is no value data or when there are
> no value sites.
yes.
>
> If a function has no value sites for a particular value kind, no value
> kind/site/data should be outputted. However, if the function does have
> value sites but no value data was recorded into any of them, in that case
> value kind, value site count, and value data count for each site should
> still be emitted.
yes -- similar to branch profile with zero counts.
> Is this the intended behavior? During merges we do check
> on the value site counts. This is done in the combineInstrProfRecords
> routine, the value site counts are compared and error is emitted if value
> site counts mismatch.
yes.
David
>
> if (Dest.ValueSites[Kind].size() != Source.ValueSites[Kind].size())
> return instrprof_error::value_site_count_mismatch;
>
> Thanks,
> -Betul
>
>> Thanks,
>> -Betul
>>
>>>
>>> David
>>>
>>>
>>>
>>>>> This also has the benefit that the V2 data will be valid for V3, so we
>>>> might even be able to get away without adding branches to check the
>>>> version difference here, which would be a convenient simplification. I
>>>> haven't thought through if that actually works though.
>>>>
>>>> ReadData can still read V2, therefore I do not understand why it should
>>>> not be valid.
>>>>
>>>>>> + // Read number of value sites.
>>>>>> + if (D + sizeof(uint32_t) > End)
>>>>>> + return data_type();
>>>>>> + uint32_t ValueSiteCount = endian::readNext<uint32_t, little,
>>>>> unaligned>(D);
>>>>>> +
>>>>>> + for (uint32_t VSite = 0; VSite < ValueSiteCount; ++VSite) { +
>>>> // Read number of value data pairs at value site.
>>>>>> + if (D + sizeof(uint32_t) > End)
>>>>>> + return data_type();
>>>>>> + uint32_t ValueDataCount = endian::readNext<uint32_t, little,
>>>>> unaligned>(D);
>>>>>> +
>>>>>> + InstrProfValueSiteRecord VSiteRecord;
>>>>>> + for (uint32_t VCount = 0; VCount < ValueDataCount; ++VCount)
>>>>>> {
>>>> + if (D + 2 * sizeof(uint64_t) > End)
>>>>>> + return data_type();
>>>>>> + uint64_t Value = endian::readNext<uint64_t, little,
>>>>> unaligned>(D);
>>>>>> + if (Kind == instrprof_value_kind::indirect_call_target) {
>> +
>>>> auto Result = HashKeyMap.find(Value);
>>>>>> + if (Result != HashKeyMap.end())
>>>>>> + Value = (uint64_t)Result->second;
>>>>
>>>>> What if it isn't found? As I mentioned earlier, I'm not very
>>>>> comfortable
>>>> with this approach to storing the strings.
>>>>
>>>> I'm not storing additional string in the file. I'm reading the existing
>>>> set of keys, add them to the string table and create a map from hash
>>>> values to const char*'s to string table entries.
>>>>
>>>>>> + }
>>>>>> + uint64_t NumTaken = endian::readNext<uint64_t, little,
>>>>> unaligned>(D);
>>>>>> + VSiteRecord.ValueData.push_back(std::make_pair(Value,
>>>>> NumTaken));
>>>>>> + }
>>>>>> +
>>>>>
>> DataBuffer[DataBuffer.size()-1].ValueSites[Kind].push_back(VSiteRecord);
>>>> This is better written as DataBuffer.back(). It also might make sense
>> to
>>>> sort these here rather than in mergeValueData - more on that later.
>>>>
>>>> Done.
>>>>
>>>>>> + }
>>>>>> + }
>>>>>> }
>>>>>> return DataBuffer;
>>>>>> }
>>>>>> @@ -379,19 +444,24 @@
>>>>>> if (HashType > IndexedInstrProf::HashT::Last)
>>>>>> return error(instrprof_error::unsupported_hash_type);
>>>>>> 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, FormatVersion)));
>>>>>> + InstrProfLookupTrait(HashType, FormatVersion, HashKeyMap))); +
>>>> for (auto Key : Index->keys()) {
>>>>>> + const char* KeyTableRef = StringTable.insertString(Key);
>>>>>> + HashKeyMap.insert(std::make_pair(ComputeHash(HashType, Key),
>>>>> KeyTableRef));
>>>>>> + }
>>>>>> // Set up our iterator for readNextRecord.
>>>>>> RecordIterator = Index->data_begin();
>>>>>> return success();
>>>>>> }
>>>>>> std::error_code IndexedInstrProfReader::getFunctionCounts(
>>>>>> - StringRef FuncName, uint64_t FuncHash, std::vector<uint64_t>
>>>>> &Counts) {
>>>>>> + StringRef FuncName, uint64_t FuncHash,
>>>>>> + std::vector<uint64_t> &Counts) {
>>>>>> +
>>>>>> auto Iter = Index->find(FuncName);
>>>>>> if (Iter == Index->end())
>>>>>> return error(instrprof_error::unknown_function);
>>>>>> @@ -411,6 +481,29 @@
>>>>>> return error(instrprof_error::hash_mismatch);
>>>>>> }
>>>>>> +std::error_code IndexedInstrProfReader::getFunctionValuesForKind( +
>>>> StringRef FuncName, uint64_t FuncHash, uint32_t ValueKind,
>>>>>> + std::vector<InstrProfValueSiteRecord> &Values) {
>>>>
>>>>> Does this function work? Won't it return an index into a string table
>>>> with no means to turn that into the actual string for indirect call
>>>> profiling?
>>>>
>>>> ValueData for indirect call targets store not indices but char*'s to
>>>> string table entries. The function works for that reason and I've
>>>> locally
>>>> verified that on my end. However, I've removed this function since it's
>>>> not in use at this time.
>>>>
>>>>>> +
>>>>>> + auto Iter = Index->find(FuncName);
>>>>>> + if (Iter == Index->end())
>>>>>> + return error(instrprof_error::unknown_function);
>>>>>> +
>>>>>> + // Found it. Look for counters with the right hash.
>>>>>> + 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 (Data[I].Hash == FuncHash) {
>>>>>> + Values = Data[I].ValueSites[ValueKind];
>>>>>> + return success();
>>>>>> + }
>>>>>> + }
>>>>>> + return error(instrprof_error::hash_mismatch);
>>>>>> +}
>>>>>> +
>>>>>> std::error_code
>>>>>> IndexedInstrProfReader::readNextRecord(InstrProfRecord &Record) {
>>>>>> // Are we out of records?
>>>>>> 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,30 @@
>>>>>> #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) {}
>>>>>> /// 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);
>>>>
>>>>> I'm not a fan of dropping const here, but I think it can come back if
>>>>> we
>>>> move the updateStringTable method anyway.
>>>>
>>>> Same here. But, I couldn't find a way to get it to work otherwise.
>>>>
>>>>>> /// 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 {
>>>>>> @@ -145,7 +146,6 @@
>>>>>> const uint64_t CountersDelta;
>>>>>> const uint64_t NamesDelta;
>>>>>> };
>>>>>> -
>>>>>> bool ShouldSwapBytes;
>>>>>> uint64_t CountersDelta;
>>>>>> uint64_t NamesDelta;
>>>>>> @@ -195,10 +195,13 @@
>>>>>> std::vector<InstrProfRecord> DataBuffer;
>>>>>> IndexedInstrProf::HashT HashType;
>>>>>> unsigned FormatVersion;
>>>>>> + const std::map<uint64_t, const char*> &HashKeyMap;
>>>>>> public:
>>>>>> - InstrProfLookupTrait(IndexedInstrProf::HashT HashType, unsigned
>>>>> FormatVersion)
>>>>>> - : HashType(HashType), FormatVersion(FormatVersion) {}
>>>>>> + InstrProfLookupTrait(IndexedInstrProf::HashT HashType,
>>>>>> + unsigned FormatVersion, std::map<uint64_t, const char*>
>>>>> &HashKeyMap)
>>>>>> + : HashType(HashType), FormatVersion(FormatVersion),
>>>>>> + HashKeyMap(HashKeyMap) {}
>>>>>> typedef ArrayRef<InstrProfRecord> data_type;
>>>>>> @@ -209,6 +212,7 @@
>>>>>> 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);
>>>>>> @@ -224,7 +228,17 @@
>>>>>> return StringRef((const char *)D, N);
>>>>>> }
>>>>>> - data_type ReadData(StringRef K, const unsigned char *D,
>> offset_type
>>>>> N);
>>>>>> + data_type ReadDataV1V2(StringRef K, const unsigned char *D,
>>>>> offset_type N);
>>>>>> + data_type ReadDataV3(StringRef K, const unsigned char *D,
>>>> offset_type
>>>>> N);
>>>>>> +
>>>>>> + data_type ReadData(StringRef K, const unsigned char *D,
>> offset_type
>>>>> N) {
>>>>>> + switch (FormatVersion) {
>>>>>> + case 1:
>>>>>> + case 2: return ReadDataV1V2(K, D, N);
>>>>>> + case 3: return ReadDataV3(K, D, N);
>>>>>> + }
>>>>>> + return data_type();
>>>>>> + }
>>>>>> };
>>>>>> typedef OnDiskIterableChainedHashTable<InstrProfLookupTrait>
>>>>>> @@ -243,6 +257,10 @@
>>>>>> uint64_t FormatVersion;
>>>>>> /// The maximal execution count among all functions.
>>>>>> uint64_t MaxFunctionCount;
>>>>>> + /// String table.
>>>>>> + InstrProfStringTable StringTable;
>>>>>> + /// Map of hash values to const char* keys in profiling data. +
>>>> std::map<uint64_t, const char*> HashKeyMap;
>>>>>> IndexedInstrProfReader(const IndexedInstrProfReader &) = delete;
>>>> IndexedInstrProfReader &operator=(const IndexedInstrProfReader &) =
>>>>> delete;
>>>>>> @@ -261,6 +279,13 @@
>>>>>> /// Fill Counts with the profile data for the given function name.
>>>> std::error_code getFunctionCounts(StringRef FuncName, uint64_t
>>>>> FuncHash,
>>>>>> std::vector<uint64_t> &Counts);
>>>>>> +
>>>>>> + /// Return value profile data for the given function name and hash
>>>>> and
>>>>>> + /// value profiling kind
>>>>>> + std::error_code getFunctionValuesForKind(StringRef FuncName, +
>>>> uint64_t FuncHash, uint32_t ValueKind,
>>>>>> + std::vector<InstrProfValueSiteRecord> &Values);
>>>>>> +
>>>>>> /// Return the maximum of all known function counts.
>>>>>> uint64_t getMaximumFunctionCount() { return MaxFunctionCount; }
>>>>>> Index: include/llvm/ProfileData/InstrProf.h
>>>>>> ===================================================================
>>>>>> ---
>>>> include/llvm/ProfileData/InstrProf.h
>>>>>> +++ include/llvm/ProfileData/InstrProf.h
>>>>>> @@ -16,10 +16,11 @@
>>>>>> #ifndef LLVM_PROFILEDATA_INSTRPROF_H_
>>>>>> #define LLVM_PROFILEDATA_INSTRPROF_H_
>>>>>> -#include "llvm/ADT/StringRef.h"
>>>>>> +#include "llvm/ADT/StringSet.h"
>>>>>> #include <cstdint>
>>>>>> -#include <system_error>
>>>>>> +#include <list>
>>>>>> #include <vector>
>>>>>> +#include <system_error>
>>>>>> namespace llvm {
>>>>>> const std::error_category &instrprof_category();
>>>>>> @@ -37,21 +38,99 @@
>>>>>> unknown_function,
>>>>>> hash_mismatch,
>>>>>> count_mismatch,
>>>>>> - counter_overflow
>>>>>> + 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
>>>>>> +};
>>>>>> +
>>>>>> +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();
>>>>>> + }
>>>>>> +};
>>>>> Is this type giving us any value over just using the StringSet<>
>>>> directly?
>>>>
>>>> This is returning const char*'s which is what's stored in the value
>> data
>>>> entries for indirect call target data. So I found wrapping it like this
>>>> very useful.
>>>>
>>>>>> +
>>>>>> +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(); }
>>>>>> + InstrProfValueSiteRecord(const InstrProfValueSiteRecord &Rec) +
>>>> : ValueData(std::move(Rec.ValueData)) {}
>>>>>> +
>>>>>> + // Sort ValueData ascending by TargetValue
>>>>>> + void sortByTargetValues() {
>>>>>> + ValueData.sort([](const ValueDataPair &left, const ValueDataPair
>>>>> &right)
>>>>>> + { return left.first < right.first; });
>>>>> clang-format and follow LLVM convention for variable names please.
>>>>
>>>> Done.
>>>>
>>>>>> + }
>>>>>> + // Sort ValueData descending by NumTaken
>>>>>> + void sortByNumTaken() {
>>>>>> + ValueData.sort([](const ValueDataPair &left, const ValueDataPair
>>>>> &right)
>>>>>> + { return left.second > right.second; });
>>>>>> + }
>>>>> This seems to be unused.
>>>>
>>>> Removed and will be re-introduced w/ the rest of the patches.
>>>>
>>>>>> + // Merge data from another InstrProfValueSiteRecord
>>>>>> + void mergeValueData(InstrProfValueSiteRecord &Input) {
>>>>>> + this->sortByTargetValues();
>>>>>> + Input.sortByTargetValues();
>>>>> It seems wasteful to re-sort these all the time. Would it make sense
>> to
>>>> sort at read time, or possibly even canonicalize the on-disk order to
>>>> already be sorted?
>>>>
>>>> I left it as is at this time. Once the API is provided one can not
>>>> guarantee the future uses of it. So using the sorting here guarantees
>>>> that
>>>> future possible usages of the API's will not break this merge
>>>> implementation.
>>>>
>>>>> I guess it might make sense for the on-disk order to be sorted by
>>>> num-taken if that's what the frontend will want, and since merging is
>>>> less
>>>> common the sort cost is okay. In any case, we should probably set it up
>>>> so
>>>> that the on disk format is always sorted in a particular way - that
>> will
>>>> remove some non-determinism and we can use it to speed up whichever
>>>> operation we think is more important.
>>>>
>>>> I'll make sure the on disk format is always sorted.
>>>>
>>>>>> + auto I = ValueData.begin();
>>>>>> + auto J = Input.ValueData.begin();
>>>>>> + while (J != Input.ValueData.end()) {
>>>>>> + while (I != ValueData.end() && I->first < J->first)
>>>>>> + ++I;
>>>>>> + if (I != ValueData.end() && I->first == J->first) {
>>>>>> + I->second += J->second;
>>>>>> + ++I;
>>>>>> + ++J;
>>>>>> + continue;
>>>>>> + }
>>>>>> + ValueData.insert(I, *J);
>>>>>> + ++J;
>>>>>> + }
>>>>> I suspect a for-loop would read better here. Also please store the
>>>> ".end()" values in variables rather than re-evaluating.
>>>>
>>>> Done.
>>>>
>>>>>> + }
>>>>>> +};
>>>>>> +
>>>>>> /// 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;
>>>>>> + // Size of vector indicates the number of value sites for a value
>>>>> kind
>>>>>> + std::vector<InstrProfValueSiteRecord>
>>>>> ValueSites[instrprof_value_kind::size];
>>>>> I don't think we're gaining much by having this be an array - I was
>>>> thinking it would be more like
>>>>> std::vector<InstrProfValueSiteRecord> IndirectCalls;
>>>>> Then when we add more value types, they can have their own variables
>>>>> and
>>>> be accessed directly. Most of the code that works with these will have
>> a
>>>> particular kind in mind, and since the value data is dependent on kind
>>>> looping over these isn't generally that useful. That is, the looping we
>>>> have now is only in the reader and writer, and I can't see the users of
>>>> the data ever doing that.
>>>>
>>>> I think, I'm leaning towards keeping an array of kinds here.
>>>>
>>>>> For the reader and writer, a switch statement over the kinds will
>> allow
>>>> us to warn if someone doesn't update somewhere when they add a new
>> kind.
>>>> For the users of profile data, Data->IndirectCalls reads a lot better
>>>> than
>>>> Data->ValueSites[instrprof_value_kind::indirect_call_target].
>>>>
>>>> I'll revisit my latest patch to allow for that.
>>>>
>>>>> It really seems like the right trade off to me.
>>>>>> + // Clear value data entries
>>>>>> + void clearValueData() {
>>>>>> + for (uint32_t Kind = instrprof_value_kind::first;
>>>>>> + Kind < instrprof_value_kind::size; ++Kind)
>>>>>> + ValueSites[Kind].clear();
>>>>>> + }
>>>>
>>>>> This is never called, it can be removed.
>>>>
>>>> Done.
>>>>
>>>>>> + void updateStringTableReferences(InstrProfStringTable &StrTable) {
>>>>>> +
>>>> Name = StrTable.insertString(Name);
>>>>>> + for (auto VSite :
>>>>> ValueSites[instrprof_value_kind::indirect_call_target])
>>>>>> + for (auto VData : VSite.ValueData)
>>>>>> + VData.first = (uint64_t)StrTable.insertString((const char
>>>>> *)VData.first);
>>>>>> + }
>>>>>> };
>>>>>> } // end namespace llvm
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>
More information about the llvm-commits
mailing list