Skip to content

Commit 35c7e45

Browse files
teresajohnsonteresajohnson
committed
[MemProf] Fix inline propagation of memprof metadata
It isn't correct to always remove memprof metadata MIBs from the original allocation call after inlining. Let's say we have the following partial call graph: C D \ / v v B E | / v v A where A contains an allocation call. If both contexts including B have the same allocation behavior, the context in the memprof metadata on the allocation will be pruned, and we will have 2 MIBs with contexts: A,B and A,E. Previously, if we inlined A into B we propagate the matching MIBs onto the inlined allocation call in B' (A,B in this case), and remove it from the original out of line allocation in A. This is correct if we have a single round of bottom up inlining. However, in the compiler we can have multiple invocations of the inliner pass (e.g. LTO). We may also inline non-bottom up with an alternative inliner such as the ModuleInliner. In that case, we could end up first inlining B into C, without having inlined A into B. The call graph then looks like: D | v C' B E \ | / v v v A If we subsequently (perhaps on a later invocation of bottom up inlining) inline A into B, the previous handling would propagate the memprof MIB context A,B up into the inlined allocation in B', and remove it from the original allocation in A. The propagation into B' is fine, however, by removing it from A's allocation, we no longer reflect the context coming from C'. To fix this, simply prevent the removal of MIB from the original allocation callsites. Note that the memprof_inline.ll test has some changes to existing checking to replace "noncold" with "notcold" in the metadata. The corresponding CHECK was accidentally commented out in the old version and thus this mistake was not previously detected. Differential Revision: https://reviews.llvm.org/D140764
1 parent 5dccea5 commit 35c7e45

File tree

3 files changed

+25
-54
lines changed

3 files changed

+25
-54
lines changed

‎llvm/lib/Transforms/Utils/InlineFunction.cpp

Lines changed: 5 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -833,11 +833,9 @@ static void updateMemprofMetadata(CallBase *CI,
833833
// Update the metadata on the inlined copy ClonedCall of a call OrigCall in the
834834
// inlined callee body, based on the callsite metadata InlinedCallsiteMD from
835835
// the call that was inlined.
836-
static void
837-
propagateMemProfHelper(const CallBase *OrigCall, CallBase *ClonedCall,
838-
MDNode *InlinedCallsiteMD,
839-
std::map<const CallBase *, std::vector<Metadata *>>
840-
&OrigCallToNewMemProfMDMap) {
836+
static void propagateMemProfHelper(const CallBase *OrigCall,
837+
CallBase *ClonedCall,
838+
MDNode *InlinedCallsiteMD) {
841839
MDNode *OrigCallsiteMD = ClonedCall->getMetadata(LLVMContext::MD_callsite);
842840
MDNode *ClonedCallsiteMD = nullptr;
843841
// Check if the call originally had callsite metadata, and update it for the
@@ -860,8 +858,6 @@ propagateMemProfHelper(const CallBase *OrigCall, CallBase *ClonedCall,
860858

861859
// New call's MIB list.
862860
std::vector<Metadata *> NewMIBList;
863-
// Updated MIB list for the original call in the out-of-line callee.
864-
std::vector<Metadata *> UpdatedOrigMIBList;
865861

866862
// For each MIB metadata, check if its call stack context starts with the
867863
// new clone's callsite metadata. If so, that MIB goes onto the cloned call in
@@ -875,21 +871,14 @@ propagateMemProfHelper(const CallBase *OrigCall, CallBase *ClonedCall,
875871
if (haveCommonPrefix(StackMD, ClonedCallsiteMD))
876872
// Add it to the cloned call's MIB list.
877873
NewMIBList.push_back(MIB);
878-
else
879-
// Keep it on the original call.
880-
UpdatedOrigMIBList.push_back(MIB);
881874
}
882875
if (NewMIBList.empty()) {
883876
removeMemProfMetadata(ClonedCall);
884877
removeCallsiteMetadata(ClonedCall);
885878
return;
886879
}
887-
if (NewMIBList.size() < OrigMemProfMD->getNumOperands()) {
888-
assert(!UpdatedOrigMIBList.empty());
889-
OrigCallToNewMemProfMDMap[OrigCall] = UpdatedOrigMIBList;
880+
if (NewMIBList.size() < OrigMemProfMD->getNumOperands())
890881
updateMemprofMetadata(ClonedCall, NewMIBList);
891-
} else
892-
OrigCallToNewMemProfMDMap[OrigCall] = {};
893882
}
894883

895884
// Update memprof related metadata (!memprof and !callsite) based on the
@@ -911,9 +900,6 @@ propagateMemProfMetadata(Function *Callee, CallBase &CB,
911900
return;
912901

913902
// Propagate metadata onto the cloned calls in the inlined callee.
914-
// Can't update the original call using the VMap since it holds a const
915-
// pointer, those will be updated in the subsequent loop.
916-
std::map<const CallBase *, std::vector<Metadata *>> OrigCallToNewMemProfMDMap;
917903
for (const auto &Entry : VMap) {
918904
// See if this is a call that has been inlined and remapped, and not
919905
// simplified away in the process.
@@ -929,27 +915,7 @@ propagateMemProfMetadata(Function *Callee, CallBase &CB,
929915
removeCallsiteMetadata(ClonedCall);
930916
continue;
931917
}
932-
propagateMemProfHelper(OrigCall, ClonedCall, CallsiteMD,
933-
OrigCallToNewMemProfMDMap);
934-
}
935-
936-
// Update memprof MD on calls within the original callee function to remove
937-
// MIB with stacks that matched the inlined context (those moved to a new
938-
// memprof MD on the inlined version of the call).
939-
for (BasicBlock &BB : *Callee) {
940-
for (Instruction &I : BB) {
941-
CallBase *Call = dyn_cast<CallBase>(&I);
942-
if (!Call || !OrigCallToNewMemProfMDMap.count(Call))
943-
continue;
944-
std::vector<Metadata *> &UpdatedMemProfMD =
945-
OrigCallToNewMemProfMDMap[Call];
946-
if (!UpdatedMemProfMD.empty())
947-
updateMemprofMetadata(Call, UpdatedMemProfMD);
948-
else {
949-
removeMemProfMetadata(Call);
950-
removeCallsiteMetadata(Call);
951-
}
952-
}
918+
propagateMemProfHelper(OrigCall, ClonedCall, CallsiteMD);
953919
}
954920
}
955921

‎llvm/test/Transforms/Inline/memprof_inline.ll

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,8 @@ target triple = "x86_64-unknown-linux-gnu"
3939
; CHECK-LABEL: define dso_local noundef ptr @_Z3foov
4040
define dso_local noundef ptr @_Z3foov() #0 !dbg !39 {
4141
entry:
42-
; CHECK: call {{.*}} @_Znam
43-
; CHECK-NOT: !memprof
44-
; CHECK-NOT: !callsite
42+
;; We should keep the original memprof metadata intact.
43+
; CHECK: call {{.*}} @_Znam{{.*}} !memprof ![[ORIGMEMPROF:[0-9]+]]
4544
%call = call noalias noundef nonnull ptr @_Znam(i64 noundef 10) #6, !dbg !42, !memprof !43, !callsite !50
4645
; CHECK-NEXT: ret
4746
ret ptr %call, !dbg !51
@@ -188,10 +187,17 @@ attributes #7 = { builtin nounwind }
188187
!43 = !{!44, !46, !48}
189188
!44 = !{!45, !"cold"}
190189
!45 = !{i64 -2458008693472584243, i64 7394638144382192936}
191-
!46 = !{!47, !"noncold"}
190+
!46 = !{!47, !"notcold"}
192191
!47 = !{i64 -2458008693472584243, i64 -8908997186479157179}
193192
!48 = !{!49, !"cold"}
194193
!49 = !{i64 -2458008693472584243, i64 -8079659623765193173}
194+
; CHECK: ![[ORIGMEMPROF]] = !{![[ORIGMIB1:[0-9]+]], ![[ORIGMIB2:[0-9]+]], ![[ORIGMIB3:[0-9]+]]}
195+
; CHECK: ![[ORIGMIB1]] = !{![[ORIGMIBSTACK1:[0-9]+]], !"cold"}
196+
; CHECK: ![[ORIGMIBSTACK1]] = !{i64 -2458008693472584243, i64 7394638144382192936}
197+
; CHECK: ![[ORIGMIB2]] = !{![[ORIGMIBSTACK2:[0-9]+]], !"notcold"}
198+
; CHECK: ![[ORIGMIBSTACK2]] = !{i64 -2458008693472584243, i64 -8908997186479157179}
199+
; CHECK: ![[ORIGMIB3]] = !{![[ORIGMIBSTACK3:[0-9]+]], !"cold"}
200+
; CHECK: ![[ORIGMIBSTACK3]] = !{i64 -2458008693472584243, i64 -8079659623765193173}
195201
!50 = !{i64 -2458008693472584243}
196202
!51 = !DILocation(line: 5, column: 3, scope: !39)
197203
!52 = distinct !DISubprogram(name: "foo2", linkageName: "_Z4foo2v", scope: !1, file: !1, line: 7, type: !40, scopeLine: 7, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !41)

‎llvm/test/Transforms/Inline/memprof_inline2.ll

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,8 @@ target triple = "x86_64-unknown-linux-gnu"
4848
; CHECK-LABEL: define dso_local noundef ptr @_Z3foov
4949
define dso_local noundef ptr @_Z3foov() #0 !dbg !39 {
5050
entry:
51-
;; We should still have memprof/callsite metadata for the non-inlined calls
52-
;; from main, but should have removed those from the inlined call in_Z4foo2v.
53-
;; CHECK: call {{.*}} @_Znam{{.*}} !memprof ![[ORIGMEMPROF:[0-9]+]]
51+
;; We should keep the original memprof metadata intact.
52+
; CHECK: call {{.*}} @_Znam{{.*}} !memprof ![[ORIGMEMPROF:[0-9]+]]
5453
%call = call noalias noundef nonnull ptr @_Znam(i64 noundef 10) #7, !dbg !42, !memprof !43, !callsite !52
5554
ret ptr %call, !dbg !53
5655
}
@@ -244,22 +243,22 @@ attributes #9 = { builtin nounwind }
244243
!43 = !{!44, !46, !48, !50}
245244
!44 = !{!45, !"cold"}
246245
!45 = !{i64 -2458008693472584243, i64 7394638144382192936}
247-
!46 = !{!47, !"noncold"}
246+
!46 = !{!47, !"notcold"}
248247
!47 = !{i64 -2458008693472584243, i64 -8908997186479157179}
249-
!48 = !{!49, !"noncold"}
248+
!48 = !{!49, !"notcold"}
250249
!49 = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -4805294506621015872}
251250
!50 = !{!51, !"cold"}
252251
!51 = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -972865200055133905}
253-
; CHECK: ![[ORIGMEMPROF]] = !{![[ORIGMIB1:[0-9]+]], ![[ORIGMIB2:[0-9]+]]}
252+
; CHECK: ![[ORIGMEMPROF]] = !{![[ORIGMIB1:[0-9]+]], ![[ORIGMIB2:[0-9]+]], ![[ORIGMIB3:[0-9]+]], ![[ORIGMIB4:[0-9]+]]}
254253
; CHECK: ![[ORIGMIB1]] = !{![[ORIGMIBSTACK1:[0-9]+]], !"cold"}
255254
; CHECK: ![[ORIGMIBSTACK1]] = !{i64 -2458008693472584243, i64 7394638144382192936}
256255
; CHECK: ![[ORIGMIB2]] = !{![[ORIGMIBSTACK2:[0-9]+]], !"notcold"}
257256
; CHECK: ![[ORIGMIBSTACK2]] = !{i64 -2458008693472584243, i64 -8908997186479157179}
258-
; CHECK: ![[NEWMEMPROF]] = !{![[NEWMIB1:[0-9]+]], ![[NEWMIB2:[0-9]+]]}
259-
; CHECK: ![[NEWMIB1]] = !{![[NEWMIBSTACK1:[0-9]+]], !"notcold"}
260-
; CHECK: ![[NEWMIBSTACK1]] = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -4805294506621015872}
261-
; CHECK: ![[NEWMIB2]] = !{![[NEWMIBSTACK2:[0-9]+]], !"cold"}
262-
; CHECK: ![[NEWMIBSTACK2]] = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -972865200055133905}
257+
; CHECK: ![[ORIGMIB3]] = !{![[ORIGMIBSTACK3:[0-9]+]], !"notcold"}
258+
; CHECK: ![[ORIGMIBSTACK3]] = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -4805294506621015872}
259+
; CHECK: ![[ORIGMIB4]] = !{![[ORIGMIBSTACK4:[0-9]+]], !"cold"}
260+
; CHECK: ![[ORIGMIBSTACK4]] = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -972865200055133905}
261+
; CHECK: ![[NEWMEMPROF]] = !{![[ORIGMIB3:[0-9]+]], ![[ORIGMIB4:[0-9]+]]}
263262
; CHECK: ![[NEWCALLSITE]] = !{i64 -2458008693472584243, i64 -8079659623765193173}
264263
!52 = !{i64 -2458008693472584243}
265264
!53 = !DILocation(line: 5, column: 3, scope: !39)

0 commit comments

Comments
 (0)