[SamplePGO][NFC] Dump function profiles in order

[hoy]

Summary

Sample profiles are stored in a string map which is basically an unordered map. Printing out profiles by simply walking the string map doesn't enforce an order. I'm sorting the map in the decreasing order of total samples to enable a more stable dump, which is good for comparing two dumps.

[wlei]

LGTM,thanks!

[wenlei]

llvm/lib/ProfileData/SampleProfReader.cpp

69–73

This is duplicated of the sorting in SampleProfileWriter::writeFuncProfiles. Can we make a standalone helper in SampleProf.h to avoid duplicate?

[hoy]

llvm/lib/ProfileData/SampleProfReader.cpp

69–73

Good call, done.

[hoy]

Addressing Wenlei's comment.

[wenlei]

lgtm, thanks.

[rupprecht]

FYI, I ran into this commit when pulling into our downstream fork because we had some tests comparing expected profile dump output. Usually labeling a commit as "NFC" is reserved for patches that have no externally visible effect, but this changes the output of llvm-profdata show. See https://lists.llvm.org/pipermail/llvm-dev/2021-June/151234.html for recent discussion. I can certainly see this being a bit of an edge case.

(No objection to the patch itself, and we didn't see any ill effect so far, just wanted to mention this in case it was unexpectedly not-NFC, or in case anyone else runs across this commit.)

[hoy]

In D108147#2951100, @rupprecht wrote:

FYI, I ran into this commit when pulling into our downstream fork because we had some tests comparing expected profile dump output. Usually labeling a commit as "NFC" is reserved for patches that have no externally visible effect, but this changes the output of llvm-profdata show. See https://lists.llvm.org/pipermail/llvm-dev/2021-June/151234.html for recent discussion. I can certainly see this being a bit of an edge case.

(No objection to the patch itself, and we didn't see any ill effect so far, just wanted to mention this in case it was unexpectedly not-NFC, or in case anyone else runs across this commit.)

Thanks for the heads-up and sorry for the inconvenience. I agree that this patch is not a NFC.

llvm/include/llvm/ProfileData/SampleProf.h

private:
   /// map will contain two entries.  One for all the samples collected
   /// in the call to bar() at line offset 1, the other for all the samples
   /// collected in the call to baz() at line offset 8.
   CallsiteSampleMap CallsiteSamples;
 };
 
 raw_ostream &operator<<(raw_ostream &OS, const FunctionSamples &FS);
 
+using NameFunctionSamples = std::pair<StringRef, const FunctionSamples *>;
+
+void sortFuncProfiles(const StringMap<FunctionSamples> &ProfileMap,
+                      std::vector<NameFunctionSamples> &SortedProfiles);
+
 /// Sort a LocationT->SampleT map by LocationT.
 ///
 /// It produces a sorted list of <LocationT, SampleT> records by ascending
 /// order of LocationT.
 template <class LocationT, class SampleT> class SampleSorter {
 public:
   using SamplesWithLoc = std::pair<const LocationT, SampleT>;
   using SamplesWithLocList = SmallVector<const SamplesWithLoc *, 20>;

llvm/lib/ProfileData/SampleProf.cpp

 }
 
 raw_ostream &llvm::sampleprof::operator<<(raw_ostream &OS,
                                           const FunctionSamples &FS) {
   FS.print(OS);
   return OS;
 }
 
+void sampleprof::sortFuncProfiles(
+    const StringMap<FunctionSamples> &ProfileMap,
+    std::vector<NameFunctionSamples> &SortedProfiles) {
+  for (const auto &I : ProfileMap) {
+    assert(I.getKey() == I.second.getNameWithContext() &&
+           "Inconsistent profile map");
+    SortedProfiles.push_back(
+        std::make_pair(I.second.getNameWithContext(), &I.second));
+  }
+  llvm::stable_sort(SortedProfiles, [](const NameFunctionSamples &A,
+                                       const NameFunctionSamples &B) {
+    if (A.second->getTotalSamples() == B.second->getTotalSamples())
+      return A.first > B.first;
+    return A.second->getTotalSamples() > B.second->getTotalSamples();
+  });
+}
+
 unsigned FunctionSamples::getOffset(const DILocation *DIL) {
   return (DIL->getLine() - DIL->getScope()->getSubprogram()->getLine()) &
       0xffff;
 }
 
 LineLocation FunctionSamples::getCallSiteIdentifier(const DILocation *DIL) {
   if (FunctionSamples::ProfileIsProbeBased)
     // In a pseudo-probe based profile, a callsite is simply represented by the

llvm/lib/ProfileData/SampleProfReader.cpp

 /// \param OS Stream to emit the output to.
 void SampleProfileReader::dumpFunctionProfile(StringRef FName,
                                               raw_ostream &OS) {
   OS << "Function: " << FName << ": " << Profiles[FName];
 }
 
 /// Dump all the function profiles found on stream \p OS.
 void SampleProfileReader::dump(raw_ostream &OS) {
-  for (const auto &I : Profiles)
-    dumpFunctionProfile(I.getKey(), OS);
+  std::vector<NameFunctionSamples> V;
+  sortFuncProfiles(Profiles, V);
+  for (const auto &I : V)
+    dumpFunctionProfile(I.first, OS);
 }
 
 /// Parse \p Input as function head.
 ///
 /// Parse one line of \p Input, and update function name in \p FName,
 /// function's total sample count in \p NumSamples, function's entry
 /// count in \p NumHeadSamples.
 ///
 /// \returns true if parsing is successful.

llvm/lib/ProfileData/SampleProfWriter.cpp

 #include <utility>
 #include <vector>
 
 using namespace llvm;
 using namespace sampleprof;
 
 std::error_code SampleProfileWriter::writeFuncProfiles(
     const StringMap<FunctionSamples> &ProfileMap) {
-  // Sort the ProfileMap by total samples.
-  typedef std::pair<StringRef, const FunctionSamples *> NameFunctionSamples;
   std::vector<NameFunctionSamples> V;
-  for (const auto &I : ProfileMap) {
-    assert(I.getKey() == I.second.getNameWithContext() &&
-           "Inconsistent profile map");
-    V.push_back(std::make_pair(I.second.getNameWithContext(), &I.second));
-  }
-  llvm::stable_sort(
-      V, [](const NameFunctionSamples &A, const NameFunctionSamples &B) {
-        if (A.second->getTotalSamples() == B.second->getTotalSamples())
-          return A.first > B.first;
-        return A.second->getTotalSamples() > B.second->getTotalSamples();
-      });
-
+  sortFuncProfiles(ProfileMap, V);
   for (const auto &I : V) {
     if (std::error_code EC = writeSample(*I.second))
       return EC;
   }
   return sampleprof_error::success;
 }
 
 std::error_code

llvm/test/tools/llvm-profdata/Inputs/profile-symbol-list.expected

10: inlined callee: inline1: 2000, 0, 1 sampled lines
     }
     No inlined callsites in this function
   10: inlined callee: inline2: 4000, 0, 1 sampled lines
     Samples collected in the function's body {
       1: 4000
     }
     No inlined callsites in this function
 }
-Function: _Z3fooi: 15422, 1220, 1 sampled lines
+Function: _Z3bari: 40602, 2874, 1 sampled lines
 Samples collected in the function's body {
-  1: 1220
+  1: 2874
 }
 No inlined callsites in this function
-Function: _Z3bari: 40602, 2874, 1 sampled lines
+Function: _Z3fooi: 15422, 1220, 1 sampled lines
 Samples collected in the function's body {
-  1: 2874
+  1: 1220
 }
 No inlined callsites in this function
 ======== Dump profile symbol list ========
 _Z3goov
 _Z3sumii
 __libc_csu_fini
 __libc_csu_init
 _dl_relocate_static_pie
 _fini
 _init
 _start
 main