[memprof] Remove MemProf format Version 1 #117357
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch removes MemProf format Version 1 now that Version 2 and 3 are working well.
|
@llvm/pr-subscribers-pgo Author: Kazu Hirata (kazutakahirata) ChangesThis patch removes MemProf format Version 1 now that Version 2 and 3 Full diff: https://github.com/llvm/llvm-project/pull/117357.diff 9 Files Affected:
diff --git a/llvm/include/llvm/ProfileData/InstrProfReader.h b/llvm/include/llvm/ProfileData/InstrProfReader.h
index 058b9a1ce02e0b..1fad2343e2c961 100644
--- a/llvm/include/llvm/ProfileData/InstrProfReader.h
+++ b/llvm/include/llvm/ProfileData/InstrProfReader.h
@@ -686,7 +686,7 @@ class IndexedMemProfReader {
// The number of elements in the radix tree array.
unsigned RadixTreeSize = 0;
- Error deserializeV12(const unsigned char *Start, const unsigned char *Ptr);
+ Error deserializeV2(const unsigned char *Start, const unsigned char *Ptr);
Error deserializeV3(const unsigned char *Start, const unsigned char *Ptr);
public:
diff --git a/llvm/include/llvm/ProfileData/MemProf.h b/llvm/include/llvm/ProfileData/MemProf.h
index f97fbd4bd64419..f4f590a4edc9fc 100644
--- a/llvm/include/llvm/ProfileData/MemProf.h
+++ b/llvm/include/llvm/ProfileData/MemProf.h
@@ -25,8 +25,6 @@ struct MemProfRecord;
// The versions of the indexed MemProf format
enum IndexedVersion : uint64_t {
- // Version 1: Added a version field to the header.
- Version1 = 1,
// Version 2: Added a call stack table.
Version2 = 2,
// Version 3: Added a radix tree for call stacks. Switched to linear IDs for
@@ -34,7 +32,7 @@ enum IndexedVersion : uint64_t {
Version3 = 3,
};
-constexpr uint64_t MinimumSupportedVersion = Version1;
+constexpr uint64_t MinimumSupportedVersion = Version2;
constexpr uint64_t MaximumSupportedVersion = Version3;
// Verify that the minimum and maximum satisfy the obvious constraint.
@@ -486,20 +484,6 @@ struct MemProfRecord {
llvm::SmallVector<std::vector<Frame>> CallSites;
MemProfRecord() = default;
- MemProfRecord(
- const IndexedMemProfRecord &Record,
- llvm::function_ref<const Frame(const FrameId Id)> IdToFrameCallback) {
- for (const IndexedAllocationInfo &IndexedAI : Record.AllocSites) {
- AllocSites.emplace_back(IndexedAI, IdToFrameCallback);
- }
- for (const ArrayRef<FrameId> Site : Record.CallSites) {
- std::vector<Frame> Frames;
- for (const FrameId Id : Site) {
- Frames.push_back(IdToFrameCallback(Id));
- }
- CallSites.push_back(Frames);
- }
- }
// Prints out the contents of the memprof record in YAML.
void print(llvm::raw_ostream &OS) const {
diff --git a/llvm/lib/ProfileData/InstrProfReader.cpp b/llvm/lib/ProfileData/InstrProfReader.cpp
index e43f3ac9f08d49..2e4ce5a2782f7e 100644
--- a/llvm/lib/ProfileData/InstrProfReader.cpp
+++ b/llvm/lib/ProfileData/InstrProfReader.cpp
@@ -1225,8 +1225,8 @@ IndexedInstrProfReader::readSummary(IndexedInstrProf::ProfVersion Version,
}
}
-Error IndexedMemProfReader::deserializeV12(const unsigned char *Start,
- const unsigned char *Ptr) {
+Error IndexedMemProfReader::deserializeV2(const unsigned char *Start,
+ const unsigned char *Ptr) {
// The value returned from RecordTableGenerator.Emit.
const uint64_t RecordTableOffset =
support::endian::readNext<uint64_t, llvm::endianness::little>(Ptr);
@@ -1322,8 +1322,7 @@ Error IndexedMemProfReader::deserialize(const unsigned char *Start,
const uint64_t FirstWord =
support::endian::readNext<uint64_t, llvm::endianness::little>(Ptr);
- if (FirstWord == memprof::Version1 || FirstWord == memprof::Version2 ||
- FirstWord == memprof::Version3) {
+ if (FirstWord == memprof::Version2 || FirstWord == memprof::Version3) {
// Everything is good. We can proceed to deserialize the rest.
Version = static_cast<memprof::IndexedVersion>(FirstWord);
} else {
@@ -1336,9 +1335,8 @@ Error IndexedMemProfReader::deserialize(const unsigned char *Start,
}
switch (Version) {
- case memprof::Version1:
case memprof::Version2:
- if (Error E = deserializeV12(Start, Ptr))
+ if (Error E = deserializeV2(Start, Ptr))
return E;
break;
case memprof::Version3:
@@ -1558,25 +1556,6 @@ Expected<InstrProfRecord> IndexedInstrProfReader::getInstrProfRecord(
return error(instrprof_error::unknown_function);
}
-static Expected<memprof::MemProfRecord>
-getMemProfRecordV0(const memprof::IndexedMemProfRecord &IndexedRecord,
- MemProfFrameHashTable &MemProfFrameTable) {
- memprof::FrameIdConverter<MemProfFrameHashTable> FrameIdConv(
- MemProfFrameTable);
-
- memprof::MemProfRecord Record =
- memprof::MemProfRecord(IndexedRecord, FrameIdConv);
-
- // Check that all frame ids were successfully converted to frames.
- if (FrameIdConv.LastUnmappedId) {
- return make_error<InstrProfError>(instrprof_error::hash_mismatch,
- "memprof frame not found for frame id " +
- Twine(*FrameIdConv.LastUnmappedId));
- }
-
- return Record;
-}
-
static Expected<memprof::MemProfRecord>
getMemProfRecordV2(const memprof::IndexedMemProfRecord &IndexedRecord,
MemProfFrameHashTable &MemProfFrameTable,
@@ -1631,11 +1610,6 @@ IndexedMemProfReader::getMemProfRecord(const uint64_t FuncNameHash) const {
const memprof::IndexedMemProfRecord &IndexedRecord = *Iter;
switch (Version) {
- case memprof::Version1:
- assert(MemProfFrameTable && "MemProfFrameTable must be available");
- assert(!MemProfCallStackTable &&
- "MemProfCallStackTable must not be available");
- return getMemProfRecordV0(IndexedRecord, *MemProfFrameTable);
case memprof::Version2:
assert(MemProfFrameTable && "MemProfFrameTable must be available");
assert(MemProfCallStackTable && "MemProfCallStackTable must be available");
diff --git a/llvm/lib/ProfileData/InstrProfWriter.cpp b/llvm/lib/ProfileData/InstrProfWriter.cpp
index d90629ad57f5b9..725ff9256fd4a0 100644
--- a/llvm/lib/ProfileData/InstrProfWriter.cpp
+++ b/llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -649,41 +649,6 @@ writeMemProfCallStackArray(
return MemProfCallStackIndexes;
}
-// Write out MemProf Version1 as follows:
-// uint64_t Version (NEW in V1)
-// uint64_t RecordTableOffset = RecordTableGenerator.Emit
-// uint64_t FramePayloadOffset = Offset for the frame payload
-// uint64_t FrameTableOffset = FrameTableGenerator.Emit
-// uint64_t Num schema entries
-// uint64_t Schema entry 0
-// uint64_t Schema entry 1
-// ....
-// uint64_t Schema entry N - 1
-// OnDiskChainedHashTable MemProfRecordData
-// OnDiskChainedHashTable MemProfFrameData
-static Error writeMemProfV1(ProfOStream &OS,
- memprof::IndexedMemProfData &MemProfData) {
- OS.write(memprof::Version1);
- uint64_t HeaderUpdatePos = OS.tell();
- 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.
-
- auto Schema = memprof::getFullSchema();
- writeMemProfSchema(OS, Schema);
-
- uint64_t RecordTableOffset =
- writeMemProfRecords(OS, MemProfData.Records, &Schema, memprof::Version1);
-
- uint64_t FramePayloadOffset = OS.tell();
- uint64_t FrameTableOffset = writeMemProfFrames(OS, MemProfData.Frames);
-
- uint64_t Header[] = {RecordTableOffset, FramePayloadOffset, FrameTableOffset};
- OS.patch({{HeaderUpdatePos, Header}});
-
- return Error::success();
-}
-
// Write out MemProf Version2 as follows:
// uint64_t Version
// uint64_t RecordTableOffset = RecordTableGenerator.Emit
@@ -805,8 +770,6 @@ static Error writeMemProf(ProfOStream &OS,
memprof::IndexedVersion MemProfVersionRequested,
bool MemProfFullSchema) {
switch (MemProfVersionRequested) {
- case memprof::Version1:
- return writeMemProfV1(OS, MemProfData);
case memprof::Version2:
return writeMemProfV2(OS, MemProfData, MemProfFullSchema);
case memprof::Version3:
diff --git a/llvm/lib/ProfileData/MemProf.cpp b/llvm/lib/ProfileData/MemProf.cpp
index 9d5ac748d7975d..9615fdf77eb27e 100644
--- a/llvm/lib/ProfileData/MemProf.cpp
+++ b/llvm/lib/ProfileData/MemProf.cpp
@@ -23,18 +23,6 @@ MemProfSchema getHotColdSchema() {
Meta::TotalLifetimeAccessDensity};
}
-static size_t serializedSizeV0(const IndexedAllocationInfo &IAI,
- const MemProfSchema &Schema) {
- size_t Size = 0;
- // The number of frames to serialize.
- Size += sizeof(uint64_t);
- // The callstack frame ids.
- Size += sizeof(FrameId) * IAI.CallStack.size();
- // The size of the payload.
- Size += PortableMemInfoBlock::serializedSize(Schema);
- return Size;
-}
-
static size_t serializedSizeV2(const IndexedAllocationInfo &IAI,
const MemProfSchema &Schema) {
size_t Size = 0;
@@ -58,8 +46,6 @@ static size_t serializedSizeV3(const IndexedAllocationInfo &IAI,
size_t IndexedAllocationInfo::serializedSize(const MemProfSchema &Schema,
IndexedVersion Version) const {
switch (Version) {
- case Version1:
- return serializedSizeV0(*this, Schema);
case Version2:
return serializedSizeV2(*this, Schema);
case Version3:
@@ -68,23 +54,6 @@ size_t IndexedAllocationInfo::serializedSize(const MemProfSchema &Schema,
llvm_unreachable("unsupported MemProf version");
}
-static size_t serializedSizeV1(const IndexedMemProfRecord &Record,
- const MemProfSchema &Schema) {
- // The number of alloc sites to serialize.
- size_t Result = sizeof(uint64_t);
- for (const IndexedAllocationInfo &N : Record.AllocSites)
- Result += N.serializedSize(Schema, Version1);
-
- // The number of callsites we have information for.
- Result += sizeof(uint64_t);
- for (const auto &Frames : Record.CallSites) {
- // The number of frame ids to serialize.
- Result += sizeof(uint64_t);
- Result += Frames.size() * sizeof(FrameId);
- }
- return Result;
-}
-
static size_t serializedSizeV2(const IndexedMemProfRecord &Record,
const MemProfSchema &Schema) {
// The number of alloc sites to serialize.
@@ -116,8 +85,6 @@ static size_t serializedSizeV3(const IndexedMemProfRecord &Record,
size_t IndexedMemProfRecord::serializedSize(const MemProfSchema &Schema,
IndexedVersion Version) const {
switch (Version) {
- case Version1:
- return serializedSizeV1(*this, Schema);
case Version2:
return serializedSizeV2(*this, Schema);
case Version3:
@@ -126,29 +93,6 @@ size_t IndexedMemProfRecord::serializedSize(const MemProfSchema &Schema,
llvm_unreachable("unsupported MemProf version");
}
-static void serializeV1(const IndexedMemProfRecord &Record,
- const MemProfSchema &Schema, raw_ostream &OS) {
- using namespace support;
-
- endian::Writer LE(OS, llvm::endianness::little);
-
- LE.write<uint64_t>(Record.AllocSites.size());
- for (const IndexedAllocationInfo &N : Record.AllocSites) {
- LE.write<uint64_t>(N.CallStack.size());
- for (const FrameId &Id : N.CallStack)
- LE.write<FrameId>(Id);
- N.Info.serialize(Schema, OS);
- }
-
- // Related contexts.
- LE.write<uint64_t>(Record.CallSites.size());
- for (const auto &Frames : Record.CallSites) {
- LE.write<uint64_t>(Frames.size());
- for (const FrameId &Id : Frames)
- LE.write<FrameId>(Id);
- }
-}
-
static void serializeV2(const IndexedMemProfRecord &Record,
const MemProfSchema &Schema, raw_ostream &OS) {
using namespace support;
@@ -195,9 +139,6 @@ void IndexedMemProfRecord::serialize(
llvm::DenseMap<CallStackId, LinearCallStackId> *MemProfCallStackIndexes)
const {
switch (Version) {
- case Version1:
- serializeV1(*this, Schema, OS);
- return;
case Version2:
serializeV2(*this, Schema, OS);
return;
@@ -208,50 +149,6 @@ void IndexedMemProfRecord::serialize(
llvm_unreachable("unsupported MemProf version");
}
-static IndexedMemProfRecord deserializeV1(const MemProfSchema &Schema,
- const unsigned char *Ptr) {
- using namespace support;
-
- IndexedMemProfRecord Record;
-
- // Read the meminfo nodes.
- const uint64_t NumNodes =
- endian::readNext<uint64_t, llvm::endianness::little>(Ptr);
- for (uint64_t I = 0; I < NumNodes; I++) {
- IndexedAllocationInfo Node;
- const uint64_t NumFrames =
- endian::readNext<uint64_t, llvm::endianness::little>(Ptr);
- for (uint64_t J = 0; J < NumFrames; J++) {
- const FrameId Id =
- endian::readNext<FrameId, llvm::endianness::little>(Ptr);
- Node.CallStack.push_back(Id);
- }
- Node.CSId = hashCallStack(Node.CallStack);
- Node.Info.deserialize(Schema, Ptr);
- Ptr += PortableMemInfoBlock::serializedSize(Schema);
- Record.AllocSites.push_back(Node);
- }
-
- // Read the callsite information.
- const uint64_t NumCtxs =
- endian::readNext<uint64_t, llvm::endianness::little>(Ptr);
- for (uint64_t J = 0; J < NumCtxs; J++) {
- const uint64_t NumFrames =
- endian::readNext<uint64_t, llvm::endianness::little>(Ptr);
- llvm::SmallVector<FrameId> Frames;
- Frames.reserve(NumFrames);
- for (uint64_t K = 0; K < NumFrames; K++) {
- const FrameId Id =
- endian::readNext<FrameId, llvm::endianness::little>(Ptr);
- Frames.push_back(Id);
- }
- Record.CallSites.push_back(Frames);
- Record.CallSiteIds.push_back(hashCallStack(Frames));
- }
-
- return Record;
-}
-
static IndexedMemProfRecord deserializeV2(const MemProfSchema &Schema,
const unsigned char *Ptr) {
using namespace support;
@@ -324,8 +221,6 @@ IndexedMemProfRecord::deserialize(const MemProfSchema &Schema,
const unsigned char *Ptr,
IndexedVersion Version) {
switch (Version) {
- case Version1:
- return deserializeV1(Schema, Ptr);
case Version2:
return deserializeV2(Schema, Ptr);
case Version3:
diff --git a/llvm/test/tools/llvm-profdata/memprof-merge-versions.test b/llvm/test/tools/llvm-profdata/memprof-merge-versions.test
index 722ef43484e7e9..0a658720162593 100644
--- a/llvm/test/tools/llvm-profdata/memprof-merge-versions.test
+++ b/llvm/test/tools/llvm-profdata/memprof-merge-versions.test
@@ -7,9 +7,6 @@ RUN: echo "1" >> %t.proftext
RUN: echo "1" >> %t.proftext
To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
-RUN: llvm-profdata merge %t.proftext %p/Inputs/basic.memprofraw --memprof-version=1 --profiled-binary %p/Inputs/basic.memprofexe -o %t.prof.v1
-RUN: llvm-profdata show %t.prof.v1 | FileCheck %s
-
RUN: llvm-profdata merge %t.proftext %p/Inputs/basic.memprofraw --memprof-version=2 --profiled-binary %p/Inputs/basic.memprofexe -o %t.prof.v2
RUN: llvm-profdata show %t.prof.v2 | FileCheck %s
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 7641a80129de35..2acf1cc34b2d8e 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -333,8 +333,7 @@ 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::Version3),
- cl::values(clEnumValN(memprof::Version1, "1", "version 1"),
- clEnumValN(memprof::Version2, "2", "version 2"),
+ cl::values(clEnumValN(memprof::Version2, "2", "version 2"),
clEnumValN(memprof::Version3, "3", "version 3")));
cl::opt<bool> MemProfFullSchema(
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index 8bd39fd71266af..f366b228e63512 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -394,21 +394,6 @@ MemInfoBlock makePartialMIB() {
return MIB;
}
-IndexedMemProfRecord makeRecord(
- std::initializer_list<std::initializer_list<::llvm::memprof::FrameId>>
- AllocFrames,
- std::initializer_list<std::initializer_list<::llvm::memprof::FrameId>>
- CallSiteFrames,
- const MemInfoBlock &Block = makeFullMIB()) {
- llvm::memprof::IndexedMemProfRecord MR;
- for (const auto &Frames : AllocFrames)
- MR.AllocSites.emplace_back(Frames, llvm::memprof::hashCallStack(Frames),
- Block);
- for (const auto &Frames : CallSiteFrames)
- MR.CallSites.push_back(Frames);
- return MR;
-}
-
IndexedMemProfRecord
makeRecordV2(std::initializer_list<::llvm::memprof::CallStackId> AllocFrames,
std::initializer_list<::llvm::memprof::CallStackId> CallSiteFrames,
@@ -456,48 +441,6 @@ MATCHER_P(EqualsRecord, Want, "") {
return true;
}
-TEST_F(InstrProfTest, test_memprof_v0) {
- ASSERT_THAT_ERROR(Writer.mergeProfileKind(InstrProfKind::MemProf),
- Succeeded());
-
- const IndexedMemProfRecord IndexedMR = makeRecord(
- /*AllocFrames=*/
- {
- {0, 1},
- {2, 3},
- },
- /*CallSiteFrames=*/{
- {4, 5},
- });
-
- memprof::IndexedMemProfData MemProfData;
- MemProfData.Frames = getFrameMapping();
- MemProfData.Records.try_emplace(0x9999, IndexedMR);
- Writer.addMemProfData(MemProfData, Err);
-
- auto Profile = Writer.writeBuffer();
- readProfile(std::move(Profile));
-
- auto RecordOr = Reader->getMemProfRecord(0x9999);
- ASSERT_THAT_ERROR(RecordOr.takeError(), Succeeded());
- const memprof::MemProfRecord &Record = RecordOr.get();
-
- std::optional<memprof::FrameId> LastUnmappedFrameId;
- auto IdToFrameCallback = [&](const memprof::FrameId Id) {
- auto Iter = MemProfData.Frames.find(Id);
- if (Iter == MemProfData.Frames.end()) {
- LastUnmappedFrameId = Id;
- return memprof::Frame(0, 0, 0, false);
- }
- return Iter->second;
- };
-
- const memprof::MemProfRecord WantRecord(IndexedMR, IdToFrameCallback);
- ASSERT_EQ(LastUnmappedFrameId, std::nullopt)
- << "could not map frame id: " << *LastUnmappedFrameId;
- EXPECT_THAT(WantRecord, EqualsRecord(Record));
-}
-
TEST_F(InstrProfTest, test_memprof_v2_full_schema) {
const MemInfoBlock MIB = makeFullMIB();
diff --git a/llvm/unittests/ProfileData/MemProfTest.cpp b/llvm/unittests/ProfileData/MemProfTest.cpp
index 79b644dc5a528d..c3f35e41b5fc74 100644
--- a/llvm/unittests/ProfileData/MemProfTest.cpp
+++ b/llvm/unittests/ProfileData/MemProfTest.cpp
@@ -268,39 +268,6 @@ TEST(MemProf, PortableWrapper) {
EXPECT_EQ(3UL, ReadBlock.getAllocCpuId());
}
-TEST(MemProf, RecordSerializationRoundTripVersion1) {
- const auto Schema = llvm::memprof::getFullSchema();
-
- MemInfoBlock Info(/*size=*/16, /*access_count=*/7, /*alloc_timestamp=*/1000,
- /*dealloc_timestamp=*/2000, /*alloc_cpu=*/3,
- /*dealloc_cpu=*/4, /*Histogram=*/0, /*HistogramSize=*/0);
-
- llvm::SmallVector<llvm::SmallVector<FrameId>> AllocCallStacks = {
- {0x123, 0x345}, {0x123, 0x567}};
-
- llvm::SmallVector<llvm::SmallVector<FrameId>> CallSites = {{0x333, 0x777}};
-
- IndexedMemProfRecord Record;
- for (const auto &ACS : AllocCallStacks) {
- // Use the same info block for both allocation sites.
- Record.AllocSites.emplace_back(ACS, llvm::memprof::hashCallStack(ACS),
- Info);
- }
- Record.CallSites.assign(CallSites);
- for (const auto &CS : CallSites)
- Record.CallSiteIds.push_back(llvm::memprof::hashCallStack(CS));
-
- std::string Buffer;
- llvm::raw_string_ostream OS(Buffer);
- Record.serialize(Schema, OS, llvm::memprof::Version1);
-
- const IndexedMemProfRecord GotRecord = IndexedMemProfRecord::deserialize(
- Schema, reinterpret_cast<const unsigned char *>(Buffer.data()),
- llvm::memprof::Version1);
-
- EXPECT_EQ(Record, GotRecord);
-}
-
TEST(MemProf, RecordSerializationRoundTripVerion2) {
const auto Schema = llvm::memprof::getFullSchema();
|
teresajohnson
approved these changes
Nov 22, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/9436 Here is the relevant piece of the build log for the reference |
|
Hi @kazutakahirata, There is the same problem on llvm-clang-x86_64-expensive-checks-ubuntu builder. |
Thank you for bringing this to my attention. I'll fix that right away. |
|
@vvereschaka Fixed with a0153ea. Thanks again! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch removes MemProf format Version 1 now that Version 2 and 3
are working well.