Skip to content

Commit 49603bd

Browse files
teresajohnsonteresajohnson
authored
Revert "[MemProf] Add ambigous memprof attribute" (#161717)
Reverts #157204 This caused issues in ThinLTO binaries because of the checking here, that didn't expect allocations needing cloning to have memprof metadata: https://github.com/llvm/llvm-project/blob/9133fc8cb04f8e45c9b46de85a8de99bf01e55c7/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp#L5572-L5582 I need to move the assert into the if check and guard by that condition. And add a more thorough test.
1 parent f1650cf commit 49603bd

File tree

4 files changed

+7
-46
lines changed

4 files changed

+7
-46
lines changed

‎llvm/include/llvm/Analysis/MemoryProfileInfo.h‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,14 +59,6 @@ LLVM_ABI std::string getAllocTypeAttributeString(AllocationType Type);
5959
/// True if the AllocTypes bitmask contains just a single type.
6060
LLVM_ABI bool hasSingleAllocType(uint8_t AllocTypes);
6161

62-
/// Removes any existing "ambiguous" memprof attribute. Called before we apply a
63-
/// specific allocation type such as "cold", "notcold", or "hot".
64-
LLVM_ABI void removeAnyExistingAmbiguousAttribute(CallBase *CB);
65-
66-
/// Adds an "ambiguous" memprof attribute to call with a matched allocation
67-
/// profile but that we haven't yet been able to disambiguate.
68-
LLVM_ABI void addAmbiguousAttribute(CallBase *CB);
69-
7062
/// Class to build a trie of call stack contexts for a particular profiled
7163
/// allocation call, along with their associated allocation types.
7264
/// The allocation will be at the root of the trie, which is then used to

‎llvm/lib/Analysis/MemoryProfileInfo.cpp‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -125,24 +125,6 @@ bool llvm::memprof::hasSingleAllocType(uint8_t AllocTypes) {
125125
return NumAllocTypes == 1;
126126
}
127127

128-
void llvm::memprof::removeAnyExistingAmbiguousAttribute(CallBase *CB) {
129-
if (!CB->hasFnAttr("memprof"))
130-
return;
131-
assert(CB->getFnAttr("memprof").getValueAsString() == "ambiguous");
132-
CB->removeFnAttr("memprof");
133-
}
134-
135-
void llvm::memprof::addAmbiguousAttribute(CallBase *CB) {
136-
// We may have an existing ambiguous attribute if we are reanalyzing
137-
// after inlining.
138-
if (CB->hasFnAttr("memprof")) {
139-
assert(CB->getFnAttr("memprof").getValueAsString() == "ambiguous");
140-
} else {
141-
auto A = llvm::Attribute::get(CB->getContext(), "memprof", "ambiguous");
142-
CB->addFnAttr(A);
143-
}
144-
}
145-
146128
void CallStackTrie::addCallStack(
147129
AllocationType AllocType, ArrayRef<uint64_t> StackIds,
148130
std::vector<ContextTotalSize> ContextSizeInfo) {
@@ -488,9 +470,6 @@ void CallStackTrie::addSingleAllocTypeAttribute(CallBase *CI, AllocationType AT,
488470
StringRef Descriptor) {
489471
auto AllocTypeString = getAllocTypeAttributeString(AT);
490472
auto A = llvm::Attribute::get(CI->getContext(), "memprof", AllocTypeString);
491-
// After inlining we may be able to convert an existing ambiguous allocation
492-
// to an unambiguous one.
493-
removeAnyExistingAmbiguousAttribute(CI);
494473
CI->addFnAttr(A);
495474
if (MemProfReportHintedSizes) {
496475
std::vector<ContextTotalSize> ContextSizeInfo;
@@ -550,7 +529,6 @@ bool CallStackTrie::buildAndAttachMIBMetadata(CallBase *CI) {
550529
assert(MIBCallStack.size() == 1 &&
551530
"Should only be left with Alloc's location in stack");
552531
CI->setMetadata(LLVMContext::MD_memprof, MDNode::get(Ctx, MIBNodes));
553-
addAmbiguousAttribute(CI);
554532
return true;
555533
}
556534
// If there exists corner case that CallStackTrie has one chain to leaf

‎llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3981,7 +3981,6 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::identifyClones(
39813981
void ModuleCallsiteContextGraph::updateAllocationCall(
39823982
CallInfo &Call, AllocationType AllocType) {
39833983
std::string AllocTypeString = getAllocTypeAttributeString(AllocType);
3984-
removeAnyExistingAmbiguousAttribute(cast<CallBase>(Call.call()));
39853984
auto A = llvm::Attribute::get(Call.call()->getFunction()->getContext(),
39863985
"memprof", AllocTypeString);
39873986
cast<CallBase>(Call.call())->addFnAttr(A);
@@ -5643,7 +5642,6 @@ bool MemProfContextDisambiguation::applyImport(Module &M) {
56435642
// clone J-1 (J==0 is the original clone and does not have a VMaps
56445643
// entry).
56455644
CBClone = cast<CallBase>((*VMaps[J - 1])[CB]);
5646-
removeAnyExistingAmbiguousAttribute(CBClone);
56475645
CBClone->addFnAttr(A);
56485646
ORE.emit(OptimizationRemark(DEBUG_TYPE, "MemprofAttribute", CBClone)
56495647
<< ore::NV("AllocationCall", CBClone) << " in clone "

‎llvm/unittests/Analysis/MemoryProfileInfoTest.cpp‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -230,8 +230,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
230230
CallBase *Call = findCall(*Func, "call");
231231
Trie.buildAndAttachMIBMetadata(Call);
232232

233-
EXPECT_TRUE(Call->hasFnAttr("memprof"));
234-
EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
233+
EXPECT_FALSE(Call->hasFnAttr("memprof"));
235234
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
236235
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
237236
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -280,8 +279,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
280279
CallBase *Call = findCall(*Func, "call");
281280
Trie.buildAndAttachMIBMetadata(Call);
282281

283-
EXPECT_TRUE(Call->hasFnAttr("memprof"));
284-
EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
282+
EXPECT_FALSE(Call->hasFnAttr("memprof"));
285283
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
286284
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
287285
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -335,8 +333,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
335333
CallBase *Call = findCall(*Func, "call");
336334
Trie.buildAndAttachMIBMetadata(Call);
337335

338-
EXPECT_TRUE(Call->hasFnAttr("memprof"));
339-
EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
336+
EXPECT_FALSE(Call->hasFnAttr("memprof"));
340337
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
341338
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
342339
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -395,8 +392,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
395392
CallBase *Call = findCall(*Func, "call");
396393
Trie.buildAndAttachMIBMetadata(Call);
397394

398-
EXPECT_TRUE(Call->hasFnAttr("memprof"));
399-
EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
395+
EXPECT_FALSE(Call->hasFnAttr("memprof"));
400396
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
401397
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
402398
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);
@@ -467,8 +463,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
467463
ASSERT_NE(Call, nullptr);
468464
Trie.buildAndAttachMIBMetadata(Call);
469465

470-
EXPECT_TRUE(Call->hasFnAttr("memprof"));
471-
EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
466+
EXPECT_FALSE(Call->hasFnAttr("memprof"));
472467
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
473468
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
474469
EXPECT_THAT(MemProfMD, MemprofMetadataEquals(ExpectedVals));
@@ -541,8 +536,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
541536
// Restore original option value.
542537
MemProfKeepAllNotColdContexts = OrigMemProfKeepAllNotColdContexts;
543538

544-
EXPECT_TRUE(Call->hasFnAttr("memprof"));
545-
EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
539+
EXPECT_FALSE(Call->hasFnAttr("memprof"));
546540
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
547541
MDNode *MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
548542
EXPECT_THAT(MemProfMD, MemprofMetadataEquals(ExpectedVals));
@@ -670,8 +664,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
670664
// The hot allocations will be converted to NotCold and pruned as they
671665
// are unnecessary to determine how to clone the cold allocation.
672666

673-
EXPECT_TRUE(Call->hasFnAttr("memprof"));
674-
EXPECT_EQ(Call->getFnAttr("memprof").getValueAsString(), "ambiguous");
667+
EXPECT_FALSE(Call->hasFnAttr("memprof"));
675668
EXPECT_TRUE(Call->hasMetadata(LLVMContext::MD_memprof));
676669
MemProfMD = Call->getMetadata(LLVMContext::MD_memprof);
677670
ASSERT_EQ(MemProfMD->getNumOperands(), 2u);

0 commit comments

Comments
 (0)