[PATCH] D10674: Value profiling - patchset 3

[Betul Buyukkurt via llvm-commits]

betulb added a comment.

> 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.

> ================

>  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



http://reviews.llvm.org/D10674