Skip to content

Conversation

Jianghibo
Copy link
Contributor

@Jianghibo Jianghibo commented Aug 27, 2025

Currently llvm-bolt does not recognize the input value for ’-print-sorted-by-order‘.

@llvmbot
Copy link
Member

llvmbot commented Aug 27, 2025

@llvm/pr-subscribers-bolt

Author: Haibo Jiang (Jianghibo)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/155613.diff

1 Files Affected:

  • (modified) bolt/lib/Passes/BinaryPasses.cpp (+4-1)
diff --git a/bolt/lib/Passes/BinaryPasses.cpp b/bolt/lib/Passes/BinaryPasses.cpp
index d7f02b9470030..176d23d275b26 100644
--- a/bolt/lib/Passes/BinaryPasses.cpp
+++ b/bolt/lib/Passes/BinaryPasses.cpp
@@ -60,7 +60,10 @@ static cl::opt<DynoStatsSortOrder> DynoStatsSortOrderOpt(
     "print-sorted-by-order",
     cl::desc("use ascending or descending order when printing functions "
              "ordered by dyno stats"),
-    cl::init(DynoStatsSortOrder::Descending), cl::cat(BoltOptCategory));
+    cl::init(DynoStatsSortOrder::Descending),
+    cl::values(clEnumValN(DynoStatsSortOrder::Ascending, "ascending", "Ascending order"),
+               clEnumValN(DynoStatsSortOrder::Descending, "descending", "Descending order")),
+    cl::cat(BoltOptCategory));
 
 cl::list<std::string>
 HotTextMoveSections("hot-text-move-sections",

Copy link

github-actions bot commented Aug 27, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@Jianghibo Jianghibo force-pushed the fix-unrecoginized-input branch from 76fff98 to 1f22678 Compare August 27, 2025 12:47
Copy link
Member

@paschalis-mpeis paschalis-mpeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it appears that descending was default with no way to set it?

llvm-bolt --dyno-stats --print-sorted-by-order=ascending ..
llvm-bolt: for the --print-sorted-by-order option: Cannot find option named 'ascending'!

llvm-bolt --dyno-stats --print-sorted-by-order=descending ..
llvm-bolt: for the --print-sorted-by-order option: Cannot find option named 'descending'!

Not sure if it's easy to add a small lit test or some text example on the PR.

@Jianghibo Jianghibo force-pushed the fix-unrecoginized-input branch from 1f22678 to 3da659b Compare September 9, 2025 07:08
@Jianghibo
Copy link
Contributor Author

Indeed it appears that descending was default with no way to set it?

llvm-bolt --dyno-stats --print-sorted-by-order=ascending ..
llvm-bolt: for the --print-sorted-by-order option: Cannot find option named 'ascending'!

llvm-bolt --dyno-stats --print-sorted-by-order=descending ..
llvm-bolt: for the --print-sorted-by-order option: Cannot find option named 'descending'!

Not sure if it's easy to add a small lit test or some text example on the PR.

Thanks for your review, added the test case.

Copy link
Member

@paschalis-mpeis paschalis-mpeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for adding a test!

Could you replace the print-sorted-by-order.fdata file with a link_fdata RUN line along with an FDATA directive? There are some examples in tests.

@Jianghibo Jianghibo force-pushed the fix-unrecoginized-input branch from 3da659b to 1a3fac6 Compare September 9, 2025 10:03
@Jianghibo Jianghibo force-pushed the fix-unrecoginized-input branch from 1a3fac6 to c6ca7ea Compare September 9, 2025 10:08
Copy link
Member

@paschalis-mpeis paschalis-mpeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Haibo,

Thanks for improving BOLT. The latest patch looks good (c6ca7ea)!
Please allow a day before merging in case there are any further reviews.

@Jianghibo
Copy link
Contributor Author

Hey Haibo,

Thanks for improving BOLT. The latest patch looks good (c6ca7ea)! Please allow a day before merging in case there are any further reviews.

Hi, I do not have permission to merge PR. Can you help me merge this PR? Thanks

@paschalis-mpeis paschalis-mpeis merged commit 3eedaa8 into llvm:main Sep 11, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants