[PATCH] D152959: [BOLT] Fix sorting functions by execution count

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev created this revision.
Herald added a reviewer: rafauler.
Herald added subscribers: treapster, ayermolo.
Herald added a reviewer: Amir.
Herald added a reviewer: maksfb.
Herald added a project: All.
spupyrev edited the summary of this revision.
spupyrev published this revision for review.
Herald added subscribers: llvm-commits, yota9.
Herald added a project: LLVM.

I noticed that `-reorder-functions=exec-count` doesn't work as expected due to
a bug in the comparison function (which isn't symmetric). It is questionable
whether anyone would want to ever use the sorting method (as sorting by say
density is much better in all cases) but it is probably better to fix the bug.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152959

Files:
  bolt/lib/Passes/ReorderFunctions.cpp


Index: bolt/lib/Passes/ReorderFunctions.cpp
===================================================================
--- bolt/lib/Passes/ReorderFunctions.cpp
+++ bolt/lib/Passes/ReorderFunctions.cpp
@@ -292,29 +292,33 @@
     break;
   case RT_EXEC_COUNT:
     {
-      std::vector<BinaryFunction *> SortedFunctions(BFs.size());
-      uint32_t Index = 0;
-      llvm::transform(llvm::make_second_range(BFs), SortedFunctions.begin(),
-                      [](BinaryFunction &BF) { return &BF; });
-      llvm::stable_sort(SortedFunctions, [&](const BinaryFunction *A,
-                                             const BinaryFunction *B) {
-        if (A->isIgnored())
-          return false;
-        const size_t PadA = opts::padFunction(*A);
-        const size_t PadB = opts::padFunction(*B);
-        if (!PadA || !PadB) {
-          if (PadA)
-            return true;
-          if (PadB)
-            return false;
-        }
-        return !A->hasProfile() &&
-               (B->hasProfile() ||
-                (A->getExecutionCount() > B->getExecutionCount()));
-      });
-      for (BinaryFunction *BF : SortedFunctions)
-        if (BF->hasProfile())
-          BF->setIndex(Index++);
+    std::vector<BinaryFunction *> SortedFunctions(BFs.size());
+    llvm::transform(llvm::make_second_range(BFs), SortedFunctions.begin(),
+                    [](BinaryFunction &BF) { return &BF; });
+    llvm::stable_sort(SortedFunctions,
+                      [&](const BinaryFunction *A, const BinaryFunction *B) {
+                        if (A->isIgnored())
+                          return false;
+                        if (B->isIgnored())
+                          return true;
+                        const size_t PadA = opts::padFunction(*A);
+                        const size_t PadB = opts::padFunction(*B);
+                        if (!PadA || !PadB) {
+                          if (PadA)
+                            return true;
+                          if (PadB)
+                            return false;
+                        }
+                        if (!A->hasProfile())
+                          return false;
+                        if (!B->hasProfile())
+                          return true;
+                        return A->getExecutionCount() > B->getExecutionCount();
+                      });
+    uint32_t Index = 0;
+    for (BinaryFunction *BF : SortedFunctions)
+      if (BF->hasProfile())
+        BF->setIndex(Index++);
     }
     break;
   case RT_HFSORT:


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D152959.531497.patch
Type: text/x-patch
Size: 2515 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230614/8f2354f2/attachment.bin>