-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[memprof] Add MemProf version #86414
[memprof] Add MemProf version #86414
Conversation
This patch adds a version field to the MemProf section of the indexed profile format, calling the new version "version 1". The existing version is called "version 0". The writer supports both versions via a command-line option: llvm-profdata merge --memprof-version=1 ... The reader supports both versions by automatically detecting the version from the header.
@llvm/pr-subscribers-pgo Author: Kazu Hirata (kazutakahirata) ChangesThis patch adds a version field to the MemProf section of the indexed The writer supports both versions via a command-line option: llvm-profdata merge --memprof-version=1 ... The reader supports both versions by automatically detecting the Full diff: https://github.com/llvm/llvm-project/pull/86414.diff 5 Files Affected:
diff --git a/llvm/include/llvm/ProfileData/InstrProfWriter.h b/llvm/include/llvm/ProfileData/InstrProfWriter.h
index f70574d1f75632..eede0464b7073c 100644
--- a/llvm/include/llvm/ProfileData/InstrProfWriter.h
+++ b/llvm/include/llvm/ProfileData/InstrProfWriter.h
@@ -75,11 +75,16 @@ class InstrProfWriter {
// deployment of newer versions of llvm-profdata.
bool WritePrevVersion = false;
+ // The version we should use to write MemProf in.
+ memprof::MemProfVersion MemProfVersionRequested;
+
public:
InstrProfWriter(bool Sparse = false,
uint64_t TemporalProfTraceReservoirSize = 0,
uint64_t MaxTemporalProfTraceLength = 0,
- bool WritePrevVersion = false);
+ bool WritePrevVersion = false,
+ memprof::MemProfVersion MemProfVersionRequested =
+ memprof::MemProfVersion0);
~InstrProfWriter();
StringMap<ProfilingData> &getProfileData() { return FunctionData; }
diff --git a/llvm/include/llvm/ProfileData/MemProf.h b/llvm/include/llvm/ProfileData/MemProf.h
index 37c19094bc2a63..81bf0445ef746e 100644
--- a/llvm/include/llvm/ProfileData/MemProf.h
+++ b/llvm/include/llvm/ProfileData/MemProf.h
@@ -15,6 +15,15 @@
namespace llvm {
namespace memprof {
+// The versions of the indexed MemProf format
+enum MemProfVersion {
+ // Version 0: This version didn't have a version field have a version in the
+ // header.
+ MemProfVersion0 = 0,
+ // Version 1: Added a version field to the header.
+ MemProfVersion1 = 1,
+};
+
enum class Meta : uint64_t {
Start = 0,
#define MIBEntryDef(NameTag, Name, Type) NameTag,
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index 31b742bca14d6f..224b5f18735257 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -24,6 +24,7 @@
#include "llvm/Support/Endian.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/SwapByteOrder.h"
#include "llvm/Support/VirtualFileSystem.h"
@@ -1230,10 +1231,38 @@ Error IndexedInstrProfReader::readHeader() {
Header->MemProfOffset);
const unsigned char *Ptr = Start + MemProfOffset;
- // The value returned from RecordTableGenerator.Emit.
- const uint64_t RecordTableOffset =
+
+ // Read the first 64-bit word, which may be RecordTableOffset in
+ // memprof::MemProfVersion0 or the MemProf version number in
+ // memprof::MemProfVersion1.
+ const uint64_t FirstWord =
support::endian::readNext<uint64_t, llvm::endianness::little,
unaligned>(Ptr);
+
+ memprof::MemProfVersion Version = memprof::MemProfVersion0;
+ if (static_cast<memprof::MemProfVersion>(FirstWord) ==
+ memprof::MemProfVersion1) {
+ // Everything is good. We can proceed to deserialize the rest.
+ Version = memprof::MemProfVersion1;
+ } else if (FirstWord >= 24) {
+ // This is a heuristic/hack to detect memprof::MemProfVersion0,
+ // which does not have a version field in the header.
+ // In memprof::MemProfVersion0, FirstWord should be RecordTableOffset,
+ // which should be at least 24 because of the MemProf header size.
+ Version = memprof::MemProfVersion0;
+ } else {
+ return make_error<InstrProfError>(
+ instrprof_error::unsupported_version,
+ formatv("MemProf version {} not supported; version 0 or 1 required",
+ FirstWord));
+ }
+
+ // The value returned from RecordTableGenerator.Emit.
+ const uint64_t RecordTableOffset =
+ Version == memprof::MemProfVersion0
+ ? FirstWord
+ : support::endian::readNext<uint64_t, llvm::endianness::little,
+ unaligned>(Ptr);
// The offset in the stream right before invoking
// FrameTableGenerator.Emit.
const uint64_t FramePayloadOffset =
diff --git a/llvm/lib/ProfileData/InstrProfWriter.cpp b/llvm/lib/ProfileData/InstrProfWriter.cpp
index d9fe88a00bdfc4..7e3da730f54443 100644
--- a/llvm/lib/ProfileData/InstrProfWriter.cpp
+++ b/llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -179,14 +179,15 @@ class InstrProfRecordWriterTrait {
} // end namespace llvm
-InstrProfWriter::InstrProfWriter(bool Sparse,
- uint64_t TemporalProfTraceReservoirSize,
- uint64_t MaxTemporalProfTraceLength,
- bool WritePrevVersion)
+InstrProfWriter::InstrProfWriter(
+ bool Sparse, uint64_t TemporalProfTraceReservoirSize,
+ uint64_t MaxTemporalProfTraceLength, bool WritePrevVersion,
+ memprof::MemProfVersion MemProfVersionRequested)
: Sparse(Sparse), MaxTemporalProfTraceLength(MaxTemporalProfTraceLength),
TemporalProfTraceReservoirSize(TemporalProfTraceReservoirSize),
InfoObj(new InstrProfRecordWriterTrait()),
- WritePrevVersion(WritePrevVersion) {}
+ WritePrevVersion(WritePrevVersion),
+ MemProfVersionRequested(MemProfVersionRequested) {}
InstrProfWriter::~InstrProfWriter() { delete InfoObj; }
@@ -528,7 +529,15 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
// OnDiskChainedHashTable MemProfFrameData
uint64_t MemProfSectionStart = 0;
if (static_cast<bool>(ProfileKind & InstrProfKind::MemProf)) {
+ assert((MemProfVersionRequested == memprof::MemProfVersion0 ||
+ MemProfVersionRequested == memprof::MemProfVersion1) &&
+ "Requested MemProf version not supported");
+
MemProfSectionStart = OS.tell();
+
+ if (MemProfVersionRequested == memprof::MemProfVersion1)
+ OS.write(memprof::MemProfVersion1);
+
OS.write(0ULL); // Reserve space for the memprof record table offset.
OS.write(0ULL); // Reserve space for the memprof frame payload offset.
OS.write(0ULL); // Reserve space for the memprof frame table offset.
@@ -570,12 +579,13 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
uint64_t FrameTableOffset = FrameTableGenerator.Emit(OS.OS, *FrameWriter);
- PatchItem PatchItems[] = {
- {MemProfSectionStart, &RecordTableOffset, 1},
- {MemProfSectionStart + sizeof(uint64_t), &FramePayloadOffset, 1},
- {MemProfSectionStart + 2 * sizeof(uint64_t), &FrameTableOffset, 1},
- };
- OS.patch(PatchItems);
+ uint64_t Header[] = {RecordTableOffset, FramePayloadOffset,
+ FrameTableOffset};
+ uint64_t HeaderUpdatePos = MemProfSectionStart;
+ if (MemProfVersionRequested == memprof::MemProfVersion1)
+ // The updates goes just after the version field.
+ HeaderUpdatePos += sizeof(uint64_t);
+ OS.patch({{HeaderUpdatePos, Header, std::size(Header)}});
}
// BinaryIdSection has two parts:
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 3a7bd061d3d23a..942a48814f14b4 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -300,6 +300,13 @@ cl::opt<bool> DoWritePrevVersion(
cl::desc("Write the previous version of indexed format, to enable "
"some forward compatibility."));
+cl::opt<memprof::MemProfVersion> MemProfVersionRequested(
+ "memprof-version", cl::Hidden, cl::sub(MergeSubcommand),
+ cl::desc("Specify the version of the memprof format to use"),
+ cl::init(memprof::MemProfVersion0),
+ cl::values(clEnumValN(memprof::MemProfVersion0, "0", "version 0"),
+ clEnumValN(memprof::MemProfVersion1, "1", "version 1")));
+
// Options specific to overlap subcommand.
cl::opt<std::string> BaseFilename(cl::Positional, cl::Required,
cl::desc("<base profile file>"),
@@ -588,7 +595,8 @@ struct WriterContext {
WriterContext(bool IsSparse, std::mutex &ErrLock,
SmallSet<instrprof_error, 4> &WriterErrorCodes,
uint64_t ReservoirSize = 0, uint64_t MaxTraceLength = 0)
- : Writer(IsSparse, ReservoirSize, MaxTraceLength, DoWritePrevVersion),
+ : Writer(IsSparse, ReservoirSize, MaxTraceLength, DoWritePrevVersion,
+ MemProfVersionRequested),
ErrLock(ErrLock), WriterErrorCodes(WriterErrorCodes) {}
};
|
✅ With the latest revision this PR passed the Python code formatter. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
Thank you for the thorough reviews! Please take a look at the revised version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment nit below. Wait to see if @snehasish has more comments though, but lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This change seems simple enough but we should have some way of testing reading / writing new and old versions before we make larger changes.
Hi @kazutakahirata, our internal Windows builder (Visual Studio 2019, Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29924 for x64) hits a compilation error when trying to build your change:
Can you take a look? |
Ack. Thank you for reporting this! |
@dyung Do you get similar errors on At
At
I am guessing that if With all that said, it's possible for your build system to terminate the build upon the first error. It may not have a chance to get to |
We do see the same errors with I was able to work around the issue with diff --git a/llvm/include/llvm/Support/FormatProviders.h b/llvm/include/llvm/Support/FormatProviders.h
index aa0773847161..a83179f014b1 100644
--- a/llvm/include/llvm/Support/FormatProviders.h
+++ b/llvm/include/llvm/Support/FormatProviders.h
@@ -126,7 +126,7 @@ protected:
template <typename T>
struct format_provider<
T, std::enable_if_t<detail::use_integral_formatter<T>::value>>
- : public detail::HelperFunctions {
+ : public llvm::detail::HelperFunctions {
private:
public:
static void format(const T &V, llvm::raw_ostream &Stream, StringRef Style) { I'm still trying to figure out how to get the compiler to find the correct overload for |
No apologies needed. I was just wondering about the differences between the two call sites of
@dyung Wow, thank you so much for looking into this. Adding
So, the |
It does not unfortunately. It seems there were two different, somewhat related issues. The first was the ambiguous llvm::detail, and the inability to resolve the call to |
I believe we found the issue with our build configuration. A while back we had disabled the option |
I'm glad you were able to restore the build. |
This patch adds a version field to the MemProf section of the indexed
profile format, calling the new version "version 1". The existing
version is called "version 0".
The writer supports both versions via a command-line option:
llvm-profdata merge --memprof-version=1 ...
The reader supports both versions by automatically detecting the
version from the header.