[PATCH] D147740: [llvm-profdata] Refactoring Sample Profile Reader to increase FDO build speed using MD5 as key to Sample Profile map

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish accepted this revision.
snehasish added a comment.

lgtm



================
Comment at: llvm/lib/ProfileData/ProfileSummaryBuilder.cpp:207
                                 !UseContextLessSummary.getNumOccurrences())) {
-    for (const auto &I : Profiles) {
-      ContextLessProfiles[I.second.getName()].merge(I.second);
-    }
+    ProfileConverter::flattenProfile(Profiles, ContextLessProfiles, true);
     ProfilesToUse = &ContextLessProfiles;
----------------
Document the parameter name /*ProfileIsCS=*/?




================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:205
   for (const auto &I : ProfileMap) {
-    assert(I.first == I.second.getContext() && "Inconsistent profile map");
-    SortedProfiles.push_back(std::make_pair(I.second.getContext(), &I.second));
+    SortedProfiles.push_back(std::make_pair(I.first, &I.second));
   }
----------------
nit: drop the make_pair in favour or an initializer list?


================
Comment at: llvm/unittests/tools/llvm-profdata/MD5CollisionTest.cpp:84
+
+    /// Samples
+    /// String1:10:1
----------------
huangjd wrote:
> huangjd wrote:
> > snehasish wrote:
> > > Can we use the text format (with some additional helper functions) here instead of the binary data? It would be hard to update in case of changes in the future.
> > See comments 
> > ```
> > ...  A unit test is required because the function
> > /// names are not printable ASCII characters.
> > ```
> This test case is very theoretical, as I can't find two printable ASCII strings with colliding MD5 (there exists for sure, but none is known yet) In this case I have to use a unit test because I cannot validate the output using llvm-lit which requires printable characters. 
> test is required because the function
> names below (i.e. String1 and String2) are not printable ASCII characters.

I see, I got confused because I thought "String1" is used in a literal sense. Perhaps enhance the comments above to note this? 






Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147740/new/

https://reviews.llvm.org/D147740