[PATCH] D14786: [llvm-profdata] Add merge() to InstrProfRecord

[Vedant Kumar via llvm-commits]

vsk added a subscriber: vsk.
vsk added a comment.

Thanks, some inline comments --


================
Comment at: include/llvm/ProfileData/InstrProf.h:323
@@ +322,3 @@
+  instrprof_error mergeValueProfData(uint32_t ValueKind,
+                                                      InstrProfRecord &Src) {
+    uint32_t ThisNumValueSites = getNumValueSites(ValueKind);
----------------
clang-format?

================
Comment at: include/llvm/ProfileData/InstrProf.h:332
@@ +331,3 @@
+            Src.getValueSitesForKind(ValueKind);
+    for (uint32_t I = 0; I < ThisNumValueSites; I++)
+      ThisSiteRecords[I].mergeValueData(OtherSiteRecords[I]);
----------------
Can you rangify this loop while we're at it?

================
Comment at: include/llvm/ProfileData/InstrProf.h:417
@@ +416,3 @@
+
+  for (size_t I = 0, E = Other.Counts.size(); I < E; ++I) {
+    if (Counts[I] + Other.Counts[I] < Counts[I])
----------------
Rangify this one too.

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:107
@@ -132,7 +106,3 @@
     // 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;
+    Where = ProfileDataMap.insert(std::make_pair(I.Hash, I)).first;
+  } else {
----------------
Is there a need to keep the iterator object around, since we have the value?


http://reviews.llvm.org/D14786