[llvm] Revert "Add unit tests for size returning new funcs in the MemProf use pass. (#105473)" (PR #106114)
Snehasish Kumar via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 26 11:23:51 PDT 2024
https://github.com/snehasish created https://github.com/llvm/llvm-project/pull/106114
This reverts commit 2e426fe8ff314c2565073e73e27fdbdf36c140a3.
>From 850482d1c827d3b6a2931f6d66e5359df65f1b67 Mon Sep 17 00:00:00 2001
From: Snehasish Kumar <snehasishk at google.com>
Date: Mon, 26 Aug 2024 18:22:12 +0000
Subject: [PATCH] Revert "Add unit tests for size returning new funcs in the
MemProf use pass. (#105473)"
This reverts commit 2e426fe8ff314c2565073e73e27fdbdf36c140a3.
---
.../llvm/ProfileData/InstrProfReader.h | 8 +-
.../Transforms/Instrumentation/MemProfiler.h | 19 +--
.../Instrumentation/MemProfiler.cpp | 42 ++---
.../Transforms/Instrumentation/CMakeLists.txt | 1 -
.../Instrumentation/MemProfilerTest.cpp | 158 ------------------
5 files changed, 29 insertions(+), 199 deletions(-)
delete mode 100644 llvm/unittests/Transforms/Instrumentation/MemProfilerTest.cpp
diff --git a/llvm/include/llvm/ProfileData/InstrProfReader.h b/llvm/include/llvm/ProfileData/InstrProfReader.h
index 95c891442fd6e9..3b307d08359980 100644
--- a/llvm/include/llvm/ProfileData/InstrProfReader.h
+++ b/llvm/include/llvm/ProfileData/InstrProfReader.h
@@ -670,11 +670,10 @@ class IndexedMemProfReader {
public:
IndexedMemProfReader() = default;
- virtual ~IndexedMemProfReader() = default;
Error deserialize(const unsigned char *Start, uint64_t MemProfOffset);
- virtual Expected<memprof::MemProfRecord>
+ Expected<memprof::MemProfRecord>
getMemProfRecord(const uint64_t FuncNameHash) const;
};
@@ -769,14 +768,11 @@ class IndexedInstrProfReader : public InstrProfReader {
uint64_t *MismatchedFuncSum = nullptr);
/// Return the memprof record for the function identified by
- /// llvm::md5(Name). Marked virtual so that unit tests can mock this function.
+ /// llvm::md5(Name).
Expected<memprof::MemProfRecord> getMemProfRecord(uint64_t FuncNameHash) {
return MemProfReader.getMemProfRecord(FuncNameHash);
}
- /// Return the underlying memprof reader.
- IndexedMemProfReader &getIndexedMemProfReader() { return MemProfReader; }
-
/// Fill Counts with the profile data for the given function name.
Error getFunctionCounts(StringRef FuncName, uint64_t FuncHash,
std::vector<uint64_t> &Counts);
diff --git a/llvm/include/llvm/Transforms/Instrumentation/MemProfiler.h b/llvm/include/llvm/Transforms/Instrumentation/MemProfiler.h
index c5d03c98f41581..f92c6b4775a2a2 100644
--- a/llvm/include/llvm/Transforms/Instrumentation/MemProfiler.h
+++ b/llvm/include/llvm/Transforms/Instrumentation/MemProfiler.h
@@ -13,15 +13,15 @@
#define LLVM_TRANSFORMS_INSTRUMENTATION_MEMPROFILER_H
#include "llvm/ADT/IntrusiveRefCntPtr.h"
-#include "llvm/IR/ModuleSummaryIndex.h"
#include "llvm/IR/PassManager.h"
-#include "llvm/ProfileData/InstrProfReader.h"
-#include "llvm/Support/VirtualFileSystem.h"
namespace llvm {
class Function;
class Module;
-class TargetLibraryInfo;
+
+namespace vfs {
+class FileSystem;
+} // namespace vfs
/// Public interface to the memory profiler pass for instrumenting code to
/// profile memory accesses.
@@ -52,17 +52,6 @@ class MemProfUsePass : public PassInfoMixin<MemProfUsePass> {
IntrusiveRefCntPtr<vfs::FileSystem> FS = nullptr);
PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
- struct AllocMatchInfo {
- uint64_t TotalSize = 0;
- AllocationType AllocType = AllocationType::None;
- bool Matched = false;
- };
-
- void
- readMemprof(Function &F, const IndexedMemProfReader &MemProfReader,
- const TargetLibraryInfo &TLI,
- std::map<uint64_t, AllocMatchInfo> &FullStackIdToAllocMatchInfo);
-
private:
std::string MemoryProfileFileName;
IntrusiveRefCntPtr<vfs::FileSystem> FS;
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
index bd10c037ecf4ad..4a43120c9a9e7f 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
@@ -39,6 +39,7 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/HashBuilder.h"
+#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/TargetParser/Triple.h"
#include "llvm/Transforms/Utils/BasicBlockUtils.h"
#include "llvm/Transforms/Utils/ModuleUtils.h"
@@ -54,7 +55,6 @@ namespace llvm {
extern cl::opt<bool> PGOWarnMissing;
extern cl::opt<bool> NoPGOWarnMismatch;
extern cl::opt<bool> NoPGOWarnMismatchComdatWeak;
-using AllocMatchInfo = ::llvm::MemProfUsePass::AllocMatchInfo;
} // namespace llvm
constexpr int LLVM_MEM_PROFILER_VERSION = 1;
@@ -148,11 +148,10 @@ static cl::opt<int> ClDebugMax("memprof-debug-max", cl::desc("Debug max inst"),
// By default disable matching of allocation profiles onto operator new that
// already explicitly pass a hot/cold hint, since we don't currently
-// override these hints anyway. Not static so that it can be set in the unit
-// test too.
-cl::opt<bool> ClMemProfMatchHotColdNew(
+// override these hints anyway.
+static cl::opt<bool> ClMemProfMatchHotColdNew(
"memprof-match-hot-cold-new",
- cl::desc(
+ cl::desc(
"Match allocation profiles onto existing hot/cold operator new calls"),
cl::Hidden, cl::init(false));
@@ -790,11 +789,17 @@ static bool isAllocationWithHotColdVariant(Function *Callee,
}
}
-void MemProfUsePass::readMemprof(
- Function &F, const IndexedMemProfReader &MemProfReader,
- const TargetLibraryInfo &TLI,
- std::map<uint64_t, AllocMatchInfo> &FullStackIdToAllocMatchInfo) {
- auto &Ctx = F.getContext();
+struct AllocMatchInfo {
+ uint64_t TotalSize = 0;
+ AllocationType AllocType = AllocationType::None;
+ bool Matched = false;
+};
+
+static void
+readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader,
+ const TargetLibraryInfo &TLI,
+ std::map<uint64_t, AllocMatchInfo> &FullStackIdToAllocMatchInfo) {
+ auto &Ctx = M.getContext();
// Previously we used getIRPGOFuncName() here. If F is local linkage,
// getIRPGOFuncName() returns FuncName with prefix 'FileName;'. But
// llvm-profdata uses FuncName in dwarf to create GUID which doesn't
@@ -805,7 +810,7 @@ void MemProfUsePass::readMemprof(
auto FuncName = F.getName();
auto FuncGUID = Function::getGUID(FuncName);
std::optional<memprof::MemProfRecord> MemProfRec;
- auto Err = MemProfReader.getMemProfRecord(FuncGUID).moveInto(MemProfRec);
+ auto Err = MemProfReader->getMemProfRecord(FuncGUID).moveInto(MemProfRec);
if (Err) {
handleAllErrors(std::move(Err), [&](const InstrProfError &IPE) {
auto Err = IPE.get();
@@ -833,8 +838,8 @@ void MemProfUsePass::readMemprof(
Twine(" Hash = ") + std::to_string(FuncGUID))
.str();
- Ctx.diagnose(DiagnosticInfoPGOProfile(F.getParent()->getName().data(),
- Msg, DS_Warning));
+ Ctx.diagnose(
+ DiagnosticInfoPGOProfile(M.getName().data(), Msg, DS_Warning));
});
return;
}
@@ -1031,15 +1036,15 @@ PreservedAnalyses MemProfUsePass::run(Module &M, ModuleAnalysisManager &AM) {
return PreservedAnalyses::all();
}
- std::unique_ptr<IndexedInstrProfReader> IndexedReader =
+ std::unique_ptr<IndexedInstrProfReader> MemProfReader =
std::move(ReaderOrErr.get());
- if (!IndexedReader) {
+ if (!MemProfReader) {
Ctx.diagnose(DiagnosticInfoPGOProfile(
- MemoryProfileFileName.data(), StringRef("Cannot get IndexedReader")));
+ MemoryProfileFileName.data(), StringRef("Cannot get MemProfReader")));
return PreservedAnalyses::all();
}
- if (!IndexedReader->hasMemoryProfile()) {
+ if (!MemProfReader->hasMemoryProfile()) {
Ctx.diagnose(DiagnosticInfoPGOProfile(MemoryProfileFileName.data(),
"Not a memory profile"));
return PreservedAnalyses::all();
@@ -1052,13 +1057,12 @@ PreservedAnalyses MemProfUsePass::run(Module &M, ModuleAnalysisManager &AM) {
// it to an allocation in the IR.
std::map<uint64_t, AllocMatchInfo> FullStackIdToAllocMatchInfo;
- const auto &MemProfReader = IndexedReader->getIndexedMemProfReader();
for (auto &F : M) {
if (F.isDeclaration())
continue;
const TargetLibraryInfo &TLI = FAM.getResult<TargetLibraryAnalysis>(F);
- readMemprof(F, MemProfReader, TLI, FullStackIdToAllocMatchInfo);
+ readMemprof(M, F, MemProfReader.get(), TLI, FullStackIdToAllocMatchInfo);
}
if (ClPrintMemProfMatchInfo) {
diff --git a/llvm/unittests/Transforms/Instrumentation/CMakeLists.txt b/llvm/unittests/Transforms/Instrumentation/CMakeLists.txt
index 1afe1c339e4335..1f249b0049d062 100644
--- a/llvm/unittests/Transforms/Instrumentation/CMakeLists.txt
+++ b/llvm/unittests/Transforms/Instrumentation/CMakeLists.txt
@@ -9,7 +9,6 @@ set(LLVM_LINK_COMPONENTS
add_llvm_unittest(InstrumentationTests
PGOInstrumentationTest.cpp
- MemProfilerTest.cpp
)
target_link_libraries(InstrumentationTests PRIVATE LLVMTestingSupport)
diff --git a/llvm/unittests/Transforms/Instrumentation/MemProfilerTest.cpp b/llvm/unittests/Transforms/Instrumentation/MemProfilerTest.cpp
deleted file mode 100644
index 844867d676e8dd..00000000000000
--- a/llvm/unittests/Transforms/Instrumentation/MemProfilerTest.cpp
+++ /dev/null
@@ -1,158 +0,0 @@
-//===- MemProfilerTest.cpp - MemProfiler unit tests ------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "llvm/Transforms/Instrumentation/MemProfiler.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/Analysis/TargetLibraryInfo.h"
-#include "llvm/AsmParser/Parser.h"
-#include "llvm/IR/Attributes.h"
-#include "llvm/IR/Metadata.h"
-#include "llvm/IR/Module.h"
-#include "llvm/IR/PassManager.h"
-#include "llvm/Passes/PassBuilder.h"
-#include "llvm/ProfileData/InstrProfReader.h"
-#include "llvm/ProfileData/MemProf.h"
-#include "llvm/ProfileData/MemProfData.inc"
-#include "llvm/Support/Error.h"
-#include "llvm/Support/SourceMgr.h"
-
-#include "gmock/gmock.h"
-#include "gtest/gtest.h"
-
-extern llvm::cl::opt<bool> ClMemProfMatchHotColdNew;
-
-namespace llvm {
-namespace memprof {
-namespace {
-
-using ::testing::Return;
-using ::testing::SizeIs;
-
-struct MemProfilerTest : public ::testing::Test {
- LLVMContext Context;
- std::unique_ptr<Module> M;
-
- MemProfilerTest() { ClMemProfMatchHotColdNew = true; }
-
- void parseAssembly(const StringRef IR) {
- SMDiagnostic Error;
- M = parseAssemblyString(IR, Error, Context);
- std::string ErrMsg;
- raw_string_ostream OS(ErrMsg);
- Error.print("", OS);
-
- // A failure here means that the test itself is buggy.
- if (!M)
- report_fatal_error(OS.str().c_str());
- }
-};
-
-// A mock memprof reader we can inject into the function we are testing.
-class MockMemProfReader : public IndexedMemProfReader {
-public:
- MOCK_METHOD(Expected<MemProfRecord>, getMemProfRecord,
- (const uint64_t FuncNameHash), (const, override));
-
- // A helper function to create mock records from frames.
- static MemProfRecord makeRecord(ArrayRef<ArrayRef<Frame>> AllocFrames) {
- MemProfRecord Record;
- MemInfoBlock Info;
- // Mimic values which will be below the cold threshold.
- Info.AllocCount = 1, Info.TotalSize = 550;
- Info.TotalLifetime = 1000 * 1000, Info.TotalLifetimeAccessDensity = 1;
- for (const auto &Callstack : AllocFrames) {
- AllocationInfo AI;
- AI.Info = PortableMemInfoBlock(Info, getHotColdSchema());
- AI.CallStack = std::vector(Callstack.begin(), Callstack.end());
- Record.AllocSites.push_back(AI);
- }
- return Record;
- }
-};
-
-TEST_F(MemProfilerTest, AnnotatesCall) {
- parseAssembly(R"IR(
- target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
- target triple = "x86_64-unknown-linux-gnu"
-
- define void @_Z3foov() !dbg !10 {
- entry:
- %c1 = call {ptr, i64} @__size_returning_new(i64 32), !dbg !13
- %c2 = call {ptr, i64} @__size_returning_new_aligned(i64 32, i64 8), !dbg !14
- %c3 = call {ptr, i64} @__size_returning_new_hot_cold(i64 32, i8 254), !dbg !15
- %c4 = call {ptr, i64} @__size_returning_new_aligned_hot_cold(i64 32, i64 8, i8 254), !dbg !16
- ret void
- }
-
- declare {ptr, i64} @__size_returning_new(i64)
- declare {ptr, i64} @__size_returning_new_aligned(i64, i64)
- declare {ptr, i64} @__size_returning_new_hot_cold(i64, i8)
- declare {ptr, i64} @__size_returning_new_aligned_hot_cold(i64, i64, i8)
-
- !llvm.dbg.cu = !{!0}
- !llvm.module.flags = !{!2, !3}
-
- !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1)
- !1 = !DIFile(filename: "mock_file.cc", directory: "mock_dir")
- !2 = !{i32 7, !"Dwarf Version", i32 5}
- !3 = !{i32 2, !"Debug Info Version", i32 3}
- !10 = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: !1, file: !1, line: 4, type: !11, scopeLine: 4, unit: !0, retainedNodes: !12)
- !11 = !DISubroutineType(types: !12)
- !12 = !{}
- !13 = !DILocation(line: 5, column: 10, scope: !10)
- !14 = !DILocation(line: 6, column: 10, scope: !10)
- !15 = !DILocation(line: 7, column: 10, scope: !10)
- !16 = !DILocation(line: 8, column: 10, scope: !10)
- )IR");
-
- auto *F = M->getFunction("_Z3foov");
- ASSERT_NE(F, nullptr);
-
- TargetLibraryInfoWrapperPass WrapperPass;
- auto &TLI = WrapperPass.getTLI(*F);
-
- auto Guid = Function::getGUID("_Z3foov");
- // All the allocation sites are in foo().
- MemProfRecord MockRecord =
- MockMemProfReader::makeRecord({{Frame(Guid, 1, 10, false)},
- {Frame(Guid, 2, 10, false)},
- {Frame(Guid, 3, 10, false)},
- {Frame(Guid, 4, 10, false)}});
- // Set up mocks for the reader.
- MockMemProfReader Reader;
- EXPECT_CALL(Reader, getMemProfRecord(Guid)).WillOnce(Return(MockRecord));
-
- MemProfUsePass Pass("/unused/profile/path");
- std::map<uint64_t, MemProfUsePass::AllocMatchInfo> Unused;
- Pass.readMemprof(*F, Reader, TLI, Unused);
-
- // Since we only have a single type of behaviour for each allocation site, we
- // only get function attributes.
- std::vector<llvm::Attribute> CallsiteAttrs;
- for (const auto &BB : *F) {
- for (const auto &I : BB) {
- if (auto *CI = dyn_cast<CallInst>(&I)) {
- if (!CI->getCalledFunction()->getName().starts_with(
- "__size_returning_new"))
- continue;
- Attribute Attr = CI->getFnAttr("memprof");
- // The attribute will be invalid if it didn't find one named memprof.
- ASSERT_TRUE(Attr.isValid());
- CallsiteAttrs.push_back(Attr);
- }
- }
- }
-
- // We match all the variants including ones with the hint since we set
- // ClMemProfMatchHotColdNew to true.
- EXPECT_THAT(CallsiteAttrs, SizeIs(4));
-}
-
-} // namespace
-} // namespace memprof
-} // namespace llvm
More information about the llvm-commits
mailing list