Revert "Add unit tests for size returning new funcs in the MemProf use pass. (#105473)" #106114
Conversation
This file contains hidden or 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
…e pass. (llvm#105473)" This reverts commit 2e426fe.
|
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-llvm-transforms Author: Snehasish Kumar (snehasish) ChangesThis reverts commit 2e426fe. Full diff: https://github.com/llvm/llvm-project/pull/106114.diff 5 Files Affected:
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
|
You can test this locally with the following command:git-clang-format --diff 7fc67b5eb53e3b99a99406ae39fba71c3f5538de 850482d1c827d3b6a2931f6d66e5359df65f1b67 --extensions h,cpp -- llvm/include/llvm/ProfileData/InstrProfReader.h llvm/include/llvm/Transforms/Instrumentation/MemProfiler.h llvm/lib/Transforms/Instrumentation/MemProfiler.cppView the diff from clang-format here.diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
index 4a43120c9a..15fd983244 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
@@ -151,7 +151,7 @@ static cl::opt<int> ClDebugMax("memprof-debug-max", cl::desc("Debug max inst"),
// 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));
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/188/builds/3388 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/3798 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/98/builds/368 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 reverts commit 2e426fe.