[PATCH] D10674: Value profiling - patchset 3

[David Li via llvm-commits]

davidxl added a comment.

Except for a couple of small issues, this version looks quite clean to me.


================
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)

================
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?

================
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?

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:99
@@ +98,3 @@
+            else LE.write<uint64_t>(V.first);
+            LE.write<uint64_t>(V.second);
+          }
----------------
Indentation?

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


http://reviews.llvm.org/D10674