Skip to content

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
@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