[PATCH] D10674: Value profiling - patchset 3

[Betul Buyukkurt via llvm-commits]

>> davidxl added a comment.
>> Except for a couple of small issues, this version looks quite clean to
> me.
>
> Thanks for the comments. I've tried to address them as inline below.
>
>> ================
>> Comment at: include/llvm/ProfileData/InstrProf.h:60
>> @@ +59,3 @@
>> +  // Get a pointer to internal storage of a string in set
>> +  const char* getStringData(StringRef Str) {
>> +    auto Result = StringValueSet.find(Str);
>> ----------------
>> betulb wrote:
>>> I've modified the data structure, however I've chosen to use an
> std::list
>>> over a linked list. I understand the presence of the cost for sites
> with
>>> no value data. I may revisit if the implementation is not preferred. I
> think usage of list makes it easier to access, sort, be iterable, and
> merge. The list of pairs implementation also allows sorting either by
> target values or numtaken. Please inform is the code is not agreeable.
>> ok, using std::list does have lots of advantages.
>> ================
>> Comment at: include/llvm/ProfileData/InstrProf.h:92
>> @@ +91,3 @@
>> +  // Merge data from another InstrProfValueSiteRecord
>> +  void mergeValueData(InstrProfValueSiteRecord &Input) {
>> +    this->sortByTargetValues();
>> ----------------
>> Looks good. (would be nicer if there is a flavor of list::unique that
> allows
>> customized handling of dup entries)
>
> A unique routine will be useful when reading in the value data from raw
> profile files. For indexed readers that I do not think it's needed. I'll
> make sure to add it when the raw profile file reader changes are put up
> for review. I'm looking forward to getting this patch in at this time.
> I've tried to address all review comments. Please take another look.
>
>> ================
>> Comment at: lib/ProfileData/InstrProfReader.cpp:397
>> @@ +396,3 @@
>> +            auto Result = HashKeyMap.find(Value);
>> +            if (Result != HashKeyMap.end())
>> +              Value = (uint64_t)Result->second;
>> ----------------
>> Error handling when it is not found?
>
> I reverted the above to check as "Result == Map.end()", whence I inserted
> a continue to drop the current value data pair and process the next.
> Setting an error_code would have been useful, but that's not implemented
> in the ReadData except as returning an empty data_type instance, which is
> then emitted as: instrprof_error::malformed.
>
>> ================
>> Comment at: lib/ProfileData/InstrProfReader.cpp:401
>> @@ +400,3 @@
>> +          uint64_t NumTaken = endian::readNext<uint64_t, little,
> unaligned>(D);
>> +          VSiteRecord.ValueData.push_back(std::make_pair(Value,
> NumTaken));
>> +        }
>> ----------------
>> Why not defining a method:
>>   IntrProfValueSiteRecord::addRecord (.. value, .. num_taken0?
>> ================
>> Comment at: lib/ProfileData/InstrProfWriter.cpp:97
>> @@ +96,3 @@
>> +            if (Kind == instrprof_value_kind::indirect_call_target) +
>            LE.write<uint64_t>(ComputeHash((const char *)V.first)); +
>        else LE.write<uint64_t>(V.first);
>> ----------------
>> I know it is extremely unlikely to have collisions with MD5, but any
> thought
>> on collision handling?
>
> The collided entries can be written as a separate table. An additional
> field in the file format may indicate if the stored target value is a hash
> or the index into the collision table.

I'd like to revisit this response. Just an alphabetical ordering index of
the collided entries in addition to the hash value, should be adequate to
resolve the collisions of the hashes in the value data. For this
StringTable should also maintain a multimap of hash ->to StringTable
entries.

>
>> ================
>> Comment at: lib/ProfileData/InstrProfWriter.cpp:99
>> @@ +98,3 @@
>> +            else LE.write<uint64_t>(V.first);
>> +            LE.write<uint64_t>(V.second);
>> +          }
>> ----------------
>> Indentation?
>
> Done.
>
>> ================
>> Comment at: lib/ProfileData/InstrProfWriter.cpp:139
>> @@ +138,3 @@
>> +InstrProfWriter::addRecord(InstrProfRecord &I) {
>> +  I.updateStringTableReferences(StringTable);
>> +  auto &ProfileDataMap = FunctionData[I.Name];
>> ----------------
>> Is this needed? I don't see StringTable is written out anywhere.
>
> Both raw and indexed readers maintain their own StringTable, so the
> indirect call target value entries arrive pointing always to the reader's
> string table. This routine would translate all the relevant value entries
> to point to the entries in the writer's StringTable.
>
> Thanks,
> -Betul
>
>> http://reviews.llvm.org/D10674
>
>
>
>
>