Skip to content

[llvm-profdata] Default to MemProf version 3 #108863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

kazutakahirata
Copy link
Contributor

It's very confusing to have support for Verion 3 but not default to
it. This patch teaches llvm-profdata to use MemProf version 3 by
default.

@llvmbot llvmbot added the PGO Profile Guided Optimizations label Sep 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2024

@llvm/pr-subscribers-pgo

Author: Kazu Hirata (kazutakahirata)

Changes

It's very confusing to have support for Verion 3 but not default to
it. This patch teaches llvm-profdata to use MemProf version 3 by
default.


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

1 Files Affected:

  • (modified) llvm/tools/llvm-profdata/llvm-profdata.cpp (+1-1)
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 5efef97cc57878..1dbe39a3f3699c 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -332,7 +332,7 @@ cl::opt<bool> DoWritePrevVersion(
 cl::opt<memprof::IndexedVersion> MemProfVersionRequested(
     "memprof-version", cl::Hidden, cl::sub(MergeSubcommand),
     cl::desc("Specify the version of the memprof format to use"),
-    cl::init(memprof::Version0),
+    cl::init(memprof::Version3),
     cl::values(clEnumValN(memprof::Version0, "0", "version 0"),
                clEnumValN(memprof::Version1, "1", "version 1"),
                clEnumValN(memprof::Version2, "2", "version 2"),

Copy link
Contributor

@teresajohnson teresajohnson left a comment

Choose a reason for hiding this comment

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

We should probably have a test that checks the default version is the one expected

It's very confusing to have support for Verion 3 but not default to
it.  This patch teaches llvm-profdata to use MemProf version 3 by
default.
@kazutakahirata
Copy link
Contributor Author

We should probably have a test that checks the default version is the one expected

I just added a test to see if the default version is V3.

I could add a test to see if the default version is the latest by scanning the help message of llvm-profdata for example, but I am wondering if that would make it harder to develop a new version of the MemProf format. During development, we might want to add a new version without bumping the default version in llvm-profdata.

@kazutakahirata kazutakahirata merged commit 75774c1 into llvm:main Oct 11, 2024
8 checks passed
@kazutakahirata kazutakahirata deleted the memprof_default_v3 branch October 11, 2024 15:56
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
It's very confusing to have support for Verion 3 but not default to
it.  This patch teaches llvm-profdata to use MemProf version 3 by
default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants