From 288ad0e450398d6c29e4db9582f28092e471591e Mon Sep 17 00:00:00 2001 From: Teresa Johnson Date: Thu, 2 Oct 2025 13:17:35 -0700 Subject: [PATCH] Reapply "[MemProf] Add ambigous memprof attribute" (#161717) Reapply llvm/llvm-project#157204 with fix and a new test for the issue it caused (the test change provoked the assert that was converted to an if condition). Also, make the application of this new attribute under an (on by default) flag, so that it can be more easily disabled if needed. Add test for the new flag. --- .../include/llvm/Analysis/MemoryProfileInfo.h | 8 ++++++ llvm/lib/Analysis/MemoryProfileInfo.cpp | 28 +++++++++++++++++++ .../IPO/MemProfContextDisambiguation.cpp | 9 ++++-- llvm/test/ThinLTO/X86/memprof-basic.ll | 5 +++- llvm/test/Transforms/PGOProfile/memprof.ll | 19 +++++++++---- .../Analysis/MemoryProfileInfoTest.cpp | 21 +++++++++----- 6 files changed, 73 insertions(+), 17 deletions(-) diff --git a/llvm/include/llvm/Analysis/MemoryProfileInfo.h b/llvm/include/llvm/Analysis/MemoryProfileInfo.h index 571caf95f275d..be690a49767b4 100644 --- a/llvm/include/llvm/Analysis/MemoryProfileInfo.h +++ b/llvm/include/llvm/Analysis/MemoryProfileInfo.h @@ -59,6 +59,14 @@ LLVM_ABI std::string getAllocTypeAttributeString(AllocationType Type); /// True if the AllocTypes bitmask contains just a single type. LLVM_ABI bool hasSingleAllocType(uint8_t AllocTypes); +/// Removes any existing "ambiguous" memprof attribute. Called before we apply a +/// specific allocation type such as "cold", "notcold", or "hot". +LLVM_ABI void removeAnyExistingAmbiguousAttribute(CallBase *CB); + +/// Adds an "ambiguous" memprof attribute to call with a matched allocation +/// profile but that we haven't yet been able to disambiguate. +LLVM_ABI void addAmbiguousAttribute(CallBase *CB); + /// Class to build a trie of call stack contexts for a particular profiled /// allocation call, along with their associated allocation types. /// The allocation will be at the root of the trie, which is then used to diff --git a/llvm/lib/Analysis/MemoryProfileInfo.cpp b/llvm/lib/Analysis/MemoryProfileInfo.cpp index 0c1f8dbd1119a..92a5b6f378aab 100644 --- a/llvm/lib/Analysis/MemoryProfileInfo.cpp +++ b/llvm/lib/Analysis/MemoryProfileInfo.cpp @@ -54,6 +54,10 @@ cl::opt MinPercentMaxColdSize( "memprof-min-percent-max-cold-size", cl::init(100), cl::Hidden, cl::desc("Min percent of max cold bytes for critical cold context")); +LLVM_ABI cl::opt MemProfUseAmbiguousAttributes( + "memprof-ambiguous-attributes", cl::init(true), cl::Hidden, + cl::desc("Apply ambiguous memprof attribute to ambiguous allocations")); + } // end namespace llvm bool llvm::memprof::metadataIncludesAllContextSizeInfo() { @@ -125,6 +129,26 @@ bool llvm::memprof::hasSingleAllocType(uint8_t AllocTypes) { return NumAllocTypes == 1; } +void llvm::memprof::removeAnyExistingAmbiguousAttribute(CallBase *CB) { + if (!CB->hasFnAttr("memprof")) + return; + assert(CB->getFnAttr("memprof").getValueAsString() == "ambiguous"); + CB->removeFnAttr("memprof"); +} + +void llvm::memprof::addAmbiguousAttribute(CallBase *CB) { + if (!MemProfUseAmbiguousAttributes) + return; + // We may have an existing ambiguous attribute if we are reanalyzing + // after inlining. + if (CB->hasFnAttr("memprof")) { + assert(CB->getFnAttr("memprof").getValueAsString() == "ambiguous"); + } else { + auto A = llvm::Attribute::get(CB->getContext(), "memprof", "ambiguous"); + CB->addFnAttr(A); + } +} + void CallStackTrie::addCallStack( AllocationType AllocType, ArrayRef StackIds, std::vector ContextSizeInfo) { @@ -470,6 +494,9 @@ void CallStackTrie::addSingleAllocTypeAttribute(CallBase *CI, AllocationType AT, StringRef Descriptor) { auto AllocTypeString = getAllocTypeAttributeString(AT); auto A = llvm::Attribute::get(CI->getContext(), "memprof", AllocTypeString); + // After inlining we may be able to convert an existing ambiguous allocation + // to an unambiguous one. + removeAnyExistingAmbiguousAttribute(CI); CI->addFnAttr(A); if (MemProfReportHintedSizes) { std::vector ContextSizeInfo; @@ -529,6 +556,7 @@ bool CallStackTrie::buildAndAttachMIBMetadata(CallBase *CI) { assert(MIBCallStack.size() == 1 && "Should only be left with Alloc's location in stack"); CI->setMetadata(LLVMContext::MD_memprof, MDNode::get(Ctx, MIBNodes)); + addAmbiguousAttribute(CI); return true; } // If there exists corner case that CallStackTrie has one chain to leaf diff --git a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp index ddb95a4184756..f9c12cf29ff58 100644 --- a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp +++ b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp @@ -3981,6 +3981,7 @@ void CallsiteContextGraph::identifyClones( void ModuleCallsiteContextGraph::updateAllocationCall( CallInfo &Call, AllocationType AllocType) { std::string AllocTypeString = getAllocTypeAttributeString(AllocType); + removeAnyExistingAmbiguousAttribute(cast(Call.call())); auto A = llvm::Attribute::get(Call.call()->getFunction()->getContext(), "memprof", AllocTypeString); cast(Call.call())->addFnAttr(A); @@ -5567,9 +5568,10 @@ bool MemProfContextDisambiguation::applyImport(Module &M) { auto *MemProfMD = I.getMetadata(LLVMContext::MD_memprof); // Include allocs that were already assigned a memprof function - // attribute in the statistics. - if (CB->getAttributes().hasFnAttr("memprof")) { - assert(!MemProfMD); + // attribute in the statistics. Only do this for those that do not have + // memprof metadata, since we add an "ambiguous" memprof attribute by + // default. + if (CB->getAttributes().hasFnAttr("memprof") && !MemProfMD) { CB->getAttributes().getFnAttr("memprof").getValueAsString() == "cold" ? AllocTypeColdThinBackend++ : AllocTypeNotColdThinBackend++; @@ -5642,6 +5644,7 @@ bool MemProfContextDisambiguation::applyImport(Module &M) { // clone J-1 (J==0 is the original clone and does not have a VMaps // entry). CBClone = cast((*VMaps[J - 1])[CB]); + removeAnyExistingAmbiguousAttribute(CBClone); CBClone->addFnAttr(A); ORE.emit(OptimizationRemark(DEBUG_TYPE, "MemprofAttribute", CBClone) << ore::NV("AllocationCall", CBClone) << " in clone " diff --git a/llvm/test/ThinLTO/X86/memprof-basic.ll b/llvm/test/ThinLTO/X86/memprof-basic.ll index 0ff0ce04452f1..537e1b8a26839 100644 --- a/llvm/test/ThinLTO/X86/memprof-basic.ll +++ b/llvm/test/ThinLTO/X86/memprof-basic.ll @@ -103,7 +103,9 @@ declare i32 @sleep() define internal ptr @_Z3barv() #0 !dbg !15 { entry: - %call = call ptr @_Znam(i64 0), !memprof !2, !callsite !7 + ;; Use an ambiguous attribute for this allocation, which is now added to such + ;; allocations during matching. It should not affect cloning. + %call = call ptr @_Znam(i64 0) #1, !memprof !2, !callsite !7 ret ptr null } @@ -125,6 +127,7 @@ entry: uselistorder ptr @_Z3foov, { 1, 0 } attributes #0 = { noinline optnone } +attributes #1 = { "memprof"="ambiguous" } !llvm.dbg.cu = !{!13} !llvm.module.flags = !{!20, !21} diff --git a/llvm/test/Transforms/PGOProfile/memprof.ll b/llvm/test/Transforms/PGOProfile/memprof.ll index c69d0311e0388..f6a89a8ceb86a 100644 --- a/llvm/test/Transforms/PGOProfile/memprof.ll +++ b/llvm/test/Transforms/PGOProfile/memprof.ll @@ -38,7 +38,7 @@ ; ALL-NOT: no profile data available for function ;; Using a memprof-only profile for memprof-use should only give memprof metadata -; RUN: opt < %s -passes='memprof-use' -pgo-warn-missing-function -S -memprof-print-match-info -stats 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,MEMPROFONLY,MEMPROFMATCHINFO,MEMPROFSTATS +; RUN: opt < %s -passes='memprof-use' -pgo-warn-missing-function -S -memprof-print-match-info -stats 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,MEMPROFONLY,MEMPROFMATCHINFO,MEMPROFSTATS,AMBIG ; There should not be any PGO metadata ; MEMPROFONLY-NOT: !prof @@ -51,10 +51,10 @@ ;; Test the same thing but by passing the memory profile through to a default ;; pipeline via -memory-profile-file=, which should cause the necessary field ;; of the PGOOptions structure to be populated with the profile filename. -; RUN: opt < %s -passes='default' -memory-profile-file=%t.memprofdata -pgo-warn-missing-function -S 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,MEMPROFONLY +; RUN: opt < %s -passes='default' -memory-profile-file=%t.memprofdata -pgo-warn-missing-function -S 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,MEMPROFONLY,AMBIG ;; Using a pgo+memprof profile for memprof-use should only give memprof metadata -; RUN: opt < %s -passes='memprof-use' -pgo-warn-missing-function -S 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,MEMPROFONLY +; RUN: opt < %s -passes='memprof-use' -pgo-warn-missing-function -S 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,MEMPROFONLY,AMBIG ;; Using a pgo-only profile for memprof-use should give an error ; RUN: not opt < %s -passes='memprof-use' -S 2>&1 | FileCheck %s --check-prefixes=MEMPROFWITHPGOONLY @@ -72,7 +72,7 @@ ;; Using a pgo+memprof profile for both memprof-use and pgo-instr-use should ;; give both memprof and pgo metadata. -; RUN: opt < %s -passes='pgo-instr-use,memprof-use' -pgo-test-profile-file=%t.pgomemprofdata -pgo-warn-missing-function -S 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,PGO +; RUN: opt < %s -passes='pgo-instr-use,memprof-use' -pgo-test-profile-file=%t.pgomemprofdata -pgo-warn-missing-function -S 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,PGO,AMBIG ;; Check that the total sizes are reported if requested. A message should be ;; emitted for the pruned context. Also check that remarks are emitted for the @@ -108,7 +108,11 @@ ;; However, with the same threshold, but hot hints not enabled, it should be ;; notcold again. -; RUN: opt < %s -passes='memprof-use' -pgo-warn-missing-function -S -memprof-min-ave-lifetime-access-density-hot-threshold=0 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL +; RUN: opt < %s -passes='memprof-use' -pgo-warn-missing-function -S -memprof-min-ave-lifetime-access-density-hot-threshold=0 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,AMBIG + +;; Test that we don't get an ambiguous memprof attribute when +;; -memprof-ambiguous-attributes is disabled. +; RUN: opt < %s -passes='memprof-use' -pgo-warn-missing-function -S -memprof-ambiguous-attributes=false 2>&1 | FileCheck %s --check-prefixes=MEMPROF,ALL,NOAMBIG ; MEMPROFMATCHINFO: MemProf notcold context with id 1093248920606587996 has total profiled size 10 is matched with 1 frames ; MEMPROFMATCHINFO: MemProf notcold context with id 5725971306423925017 has total profiled size 10 is matched with 1 frames @@ -140,7 +144,7 @@ target triple = "x86_64-unknown-linux-gnu" ; PGO: !prof define dso_local noundef ptr @_Z3foov() #0 !dbg !10 { entry: - ; MEMPROF: call {{.*}} @_Znam{{.*}} !memprof ![[M1:[0-9]+]], !callsite ![[C1:[0-9]+]] + ; MEMPROF: call {{.*}} @_Znam{{.*}} #[[A0:[0-9]+]]{{.*}} !memprof ![[M1:[0-9]+]], !callsite ![[C1:[0-9]+]] ; MEMPROFNOCOLINFO: call {{.*}} @_Znam{{.*}} !memprof ![[M1:[0-9]+]], !callsite ![[C1:[0-9]+]] %call = call noalias noundef nonnull ptr @_Znam(i64 noundef 10) #6, !dbg !13 ret ptr %call, !dbg !14 @@ -364,6 +368,9 @@ for.end: ; preds = %for.cond ret i32 0, !dbg !103 } +;; We optionally apply an ambiguous memprof attribute to ambiguous allocations +; AMBIG: #[[A0]] = { builtin allocsize(0) "memprof"="ambiguous" } +; NOAMBIG: #[[A0]] = { builtin allocsize(0) } ; MEMPROF: #[[A1]] = { builtin allocsize(0) "memprof"="notcold" } ; MEMPROF: #[[A2]] = { builtin allocsize(0) "memprof"="cold" } ; MEMPROF: ![[M1]] = !{![[MIB1:[0-9]+]], ![[MIB2:[0-9]+]], ![[MIB3:[0-9]+]], ![[MIB4:[0-9]+]]} diff --git a/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp b/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp index d8457a30fd2f7..d1c0f643f5cd7 100644 --- a/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp +++ b/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp @@ -230,7 +230,8 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef) CallBase *Call = findCall(*Func, "call"); Trie.buildAndAttachMIBMetadata(Call); - EXPECT_FALSE(Call->hasFnAttr("memprof")); + EXPECT_TRUE(Call->hasFnAttr("memprof")); + EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous"); EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof)); MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof); ASSERT_EQ(MemProfMD->getNumOperands(), 2u); @@ -279,7 +280,8 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef) CallBase *Call = findCall(*Func, "call"); Trie.buildAndAttachMIBMetadata(Call); - EXPECT_FALSE(Call->hasFnAttr("memprof")); + EXPECT_TRUE(Call->hasFnAttr("memprof")); + EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous"); EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof)); MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof); ASSERT_EQ(MemProfMD->getNumOperands(), 2u); @@ -333,7 +335,8 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef) CallBase *Call = findCall(*Func, "call"); Trie.buildAndAttachMIBMetadata(Call); - EXPECT_FALSE(Call->hasFnAttr("memprof")); + EXPECT_TRUE(Call->hasFnAttr("memprof")); + EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous"); EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof)); MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof); ASSERT_EQ(MemProfMD->getNumOperands(), 2u); @@ -392,7 +395,8 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef) CallBase *Call = findCall(*Func, "call"); Trie.buildAndAttachMIBMetadata(Call); - EXPECT_FALSE(Call->hasFnAttr("memprof")); + EXPECT_TRUE(Call->hasFnAttr("memprof")); + EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous"); EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof)); MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof); ASSERT_EQ(MemProfMD->getNumOperands(), 2u); @@ -463,7 +467,8 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef) ASSERT_NE(Call, nullptr); Trie.buildAndAttachMIBMetadata(Call); - EXPECT_FALSE(Call->hasFnAttr("memprof")); + EXPECT_TRUE(Call->hasFnAttr("memprof")); + EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous"); EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof)); MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof); EXPECT_THAT(MemProfMD, MemprofMetadataEquals(ExpectedVals)); @@ -536,7 +541,8 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef) // Restore original option value. MemProfKeepAllNotColdContexts = OrigMemProfKeepAllNotColdContexts; - EXPECT_FALSE(Call->hasFnAttr("memprof")); + EXPECT_TRUE(Call->hasFnAttr("memprof")); + EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous"); EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof)); MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof); EXPECT_THAT(MemProfMD, MemprofMetadataEquals(ExpectedVals)); @@ -664,7 +670,8 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef) // The hot allocations will be converted to NotCold and pruned as they // are unnecessary to determine how to clone the cold allocation. - EXPECT_FALSE(Call->hasFnAttr("memprof")); + EXPECT_TRUE(Call->hasFnAttr("memprof")); + EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous"); EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof)); MemProfMD = Call->getMetadata(LLVMContext::MD_memprof); ASSERT_EQ(MemProfMD->getNumOperands(), 2u);