[llvm] [memprof] Remove MemProf format Version 1 (PR #117357)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 22 10:25:24 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-pgo
Author: Kazu Hirata (kazutakahirata)
<details>
<summary>Changes</summary>
This patch removes MemProf format Version 1 now that Version 2 and 3
are working well.
---
Full diff: https://github.com/llvm/llvm-project/pull/117357.diff
9 Files Affected:
- (modified) llvm/include/llvm/ProfileData/InstrProfReader.h (+1-1)
- (modified) llvm/include/llvm/ProfileData/MemProf.h (+1-17)
- (modified) llvm/lib/ProfileData/InstrProfReader.cpp (+4-30)
- (modified) llvm/lib/ProfileData/InstrProfWriter.cpp (-37)
- (modified) llvm/lib/ProfileData/MemProf.cpp (-105)
- (modified) llvm/test/tools/llvm-profdata/memprof-merge-versions.test (-3)
- (modified) llvm/tools/llvm-profdata/llvm-profdata.cpp (+1-2)
- (modified) llvm/unittests/ProfileData/InstrProfTest.cpp (-57)
- (modified) llvm/unittests/ProfileData/MemProfTest.cpp (-33)
``````````diff
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();
``````````
</details>
https://github.com/llvm/llvm-project/pull/117357
More information about the llvm-commits
mailing list