From 35c7e457e8b00a4772b00b0100748f9a517ea0c2 Mon Sep 17 00:00:00 2001 From: Teresa Johnson Date: Thu, 29 Dec 2022 12:11:38 -0800 Subject: [PATCH] [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 --- llvm/lib/Transforms/Utils/InlineFunction.cpp | 44 +++----------------------- llvm/test/Transforms/Inline/memprof_inline.ll | 14 +++++--- llvm/test/Transforms/Inline/memprof_inline2.ll | 21 ++++++------ 3 files changed, 25 insertions(+), 54 deletions(-) diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp index bde9b91..a429b0d 100644 --- a/llvm/lib/Transforms/Utils/InlineFunction.cpp +++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp @@ -833,11 +833,9 @@ static void updateMemprofMetadata(CallBase *CI, // Update the metadata on the inlined copy ClonedCall of a call OrigCall in the // inlined callee body, based on the callsite metadata InlinedCallsiteMD from // the call that was inlined. -static void -propagateMemProfHelper(const CallBase *OrigCall, CallBase *ClonedCall, - MDNode *InlinedCallsiteMD, - std::map> - &OrigCallToNewMemProfMDMap) { +static void propagateMemProfHelper(const CallBase *OrigCall, + CallBase *ClonedCall, + MDNode *InlinedCallsiteMD) { MDNode *OrigCallsiteMD = ClonedCall->getMetadata(LLVMContext::MD_callsite); MDNode *ClonedCallsiteMD = nullptr; // Check if the call originally had callsite metadata, and update it for the @@ -860,8 +858,6 @@ propagateMemProfHelper(const CallBase *OrigCall, CallBase *ClonedCall, // New call's MIB list. std::vector NewMIBList; - // Updated MIB list for the original call in the out-of-line callee. - std::vector UpdatedOrigMIBList; // For each MIB metadata, check if its call stack context starts with the // 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, if (haveCommonPrefix(StackMD, ClonedCallsiteMD)) // Add it to the cloned call's MIB list. NewMIBList.push_back(MIB); - else - // Keep it on the original call. - UpdatedOrigMIBList.push_back(MIB); } if (NewMIBList.empty()) { removeMemProfMetadata(ClonedCall); removeCallsiteMetadata(ClonedCall); return; } - if (NewMIBList.size() < OrigMemProfMD->getNumOperands()) { - assert(!UpdatedOrigMIBList.empty()); - OrigCallToNewMemProfMDMap[OrigCall] = UpdatedOrigMIBList; + if (NewMIBList.size() < OrigMemProfMD->getNumOperands()) updateMemprofMetadata(ClonedCall, NewMIBList); - } else - OrigCallToNewMemProfMDMap[OrigCall] = {}; } // Update memprof related metadata (!memprof and !callsite) based on the @@ -911,9 +900,6 @@ propagateMemProfMetadata(Function *Callee, CallBase &CB, return; // Propagate metadata onto the cloned calls in the inlined callee. - // Can't update the original call using the VMap since it holds a const - // pointer, those will be updated in the subsequent loop. - std::map> OrigCallToNewMemProfMDMap; for (const auto &Entry : VMap) { // See if this is a call that has been inlined and remapped, and not // simplified away in the process. @@ -929,27 +915,7 @@ propagateMemProfMetadata(Function *Callee, CallBase &CB, removeCallsiteMetadata(ClonedCall); continue; } - propagateMemProfHelper(OrigCall, ClonedCall, CallsiteMD, - OrigCallToNewMemProfMDMap); - } - - // Update memprof MD on calls within the original callee function to remove - // MIB with stacks that matched the inlined context (those moved to a new - // memprof MD on the inlined version of the call). - for (BasicBlock &BB : *Callee) { - for (Instruction &I : BB) { - CallBase *Call = dyn_cast(&I); - if (!Call || !OrigCallToNewMemProfMDMap.count(Call)) - continue; - std::vector &UpdatedMemProfMD = - OrigCallToNewMemProfMDMap[Call]; - if (!UpdatedMemProfMD.empty()) - updateMemprofMetadata(Call, UpdatedMemProfMD); - else { - removeMemProfMetadata(Call); - removeCallsiteMetadata(Call); - } - } + propagateMemProfHelper(OrigCall, ClonedCall, CallsiteMD); } } diff --git a/llvm/test/Transforms/Inline/memprof_inline.ll b/llvm/test/Transforms/Inline/memprof_inline.ll index 20b914d..7383264 100644 --- a/llvm/test/Transforms/Inline/memprof_inline.ll +++ b/llvm/test/Transforms/Inline/memprof_inline.ll @@ -39,9 +39,8 @@ target triple = "x86_64-unknown-linux-gnu" ; CHECK-LABEL: define dso_local noundef ptr @_Z3foov define dso_local noundef ptr @_Z3foov() #0 !dbg !39 { entry: - ; CHECK: call {{.*}} @_Znam - ; CHECK-NOT: !memprof - ; CHECK-NOT: !callsite + ;; We should keep the original memprof metadata intact. + ; CHECK: call {{.*}} @_Znam{{.*}} !memprof ![[ORIGMEMPROF:[0-9]+]] %call = call noalias noundef nonnull ptr @_Znam(i64 noundef 10) #6, !dbg !42, !memprof !43, !callsite !50 ; CHECK-NEXT: ret ret ptr %call, !dbg !51 @@ -188,10 +187,17 @@ attributes #7 = { builtin nounwind } !43 = !{!44, !46, !48} !44 = !{!45, !"cold"} !45 = !{i64 -2458008693472584243, i64 7394638144382192936} -!46 = !{!47, !"noncold"} +!46 = !{!47, !"notcold"} !47 = !{i64 -2458008693472584243, i64 -8908997186479157179} !48 = !{!49, !"cold"} !49 = !{i64 -2458008693472584243, i64 -8079659623765193173} +; CHECK: ![[ORIGMEMPROF]] = !{![[ORIGMIB1:[0-9]+]], ![[ORIGMIB2:[0-9]+]], ![[ORIGMIB3:[0-9]+]]} +; CHECK: ![[ORIGMIB1]] = !{![[ORIGMIBSTACK1:[0-9]+]], !"cold"} +; CHECK: ![[ORIGMIBSTACK1]] = !{i64 -2458008693472584243, i64 7394638144382192936} +; CHECK: ![[ORIGMIB2]] = !{![[ORIGMIBSTACK2:[0-9]+]], !"notcold"} +; CHECK: ![[ORIGMIBSTACK2]] = !{i64 -2458008693472584243, i64 -8908997186479157179} +; CHECK: ![[ORIGMIB3]] = !{![[ORIGMIBSTACK3:[0-9]+]], !"cold"} +; CHECK: ![[ORIGMIBSTACK3]] = !{i64 -2458008693472584243, i64 -8079659623765193173} !50 = !{i64 -2458008693472584243} !51 = !DILocation(line: 5, column: 3, scope: !39) !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) diff --git a/llvm/test/Transforms/Inline/memprof_inline2.ll b/llvm/test/Transforms/Inline/memprof_inline2.ll index 4d9e2cb..625971c 100644 --- a/llvm/test/Transforms/Inline/memprof_inline2.ll +++ b/llvm/test/Transforms/Inline/memprof_inline2.ll @@ -48,9 +48,8 @@ target triple = "x86_64-unknown-linux-gnu" ; CHECK-LABEL: define dso_local noundef ptr @_Z3foov define dso_local noundef ptr @_Z3foov() #0 !dbg !39 { entry: - ;; We should still have memprof/callsite metadata for the non-inlined calls - ;; from main, but should have removed those from the inlined call in_Z4foo2v. - ;; CHECK: call {{.*}} @_Znam{{.*}} !memprof ![[ORIGMEMPROF:[0-9]+]] + ;; We should keep the original memprof metadata intact. + ; CHECK: call {{.*}} @_Znam{{.*}} !memprof ![[ORIGMEMPROF:[0-9]+]] %call = call noalias noundef nonnull ptr @_Znam(i64 noundef 10) #7, !dbg !42, !memprof !43, !callsite !52 ret ptr %call, !dbg !53 } @@ -244,22 +243,22 @@ attributes #9 = { builtin nounwind } !43 = !{!44, !46, !48, !50} !44 = !{!45, !"cold"} !45 = !{i64 -2458008693472584243, i64 7394638144382192936} -!46 = !{!47, !"noncold"} +!46 = !{!47, !"notcold"} !47 = !{i64 -2458008693472584243, i64 -8908997186479157179} -!48 = !{!49, !"noncold"} +!48 = !{!49, !"notcold"} !49 = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -4805294506621015872} !50 = !{!51, !"cold"} !51 = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -972865200055133905} -; CHECK: ![[ORIGMEMPROF]] = !{![[ORIGMIB1:[0-9]+]], ![[ORIGMIB2:[0-9]+]]} +; CHECK: ![[ORIGMEMPROF]] = !{![[ORIGMIB1:[0-9]+]], ![[ORIGMIB2:[0-9]+]], ![[ORIGMIB3:[0-9]+]], ![[ORIGMIB4:[0-9]+]]} ; CHECK: ![[ORIGMIB1]] = !{![[ORIGMIBSTACK1:[0-9]+]], !"cold"} ; CHECK: ![[ORIGMIBSTACK1]] = !{i64 -2458008693472584243, i64 7394638144382192936} ; CHECK: ![[ORIGMIB2]] = !{![[ORIGMIBSTACK2:[0-9]+]], !"notcold"} ; CHECK: ![[ORIGMIBSTACK2]] = !{i64 -2458008693472584243, i64 -8908997186479157179} -; CHECK: ![[NEWMEMPROF]] = !{![[NEWMIB1:[0-9]+]], ![[NEWMIB2:[0-9]+]]} -; CHECK: ![[NEWMIB1]] = !{![[NEWMIBSTACK1:[0-9]+]], !"notcold"} -; CHECK: ![[NEWMIBSTACK1]] = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -4805294506621015872} -; CHECK: ![[NEWMIB2]] = !{![[NEWMIBSTACK2:[0-9]+]], !"cold"} -; CHECK: ![[NEWMIBSTACK2]] = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -972865200055133905} +; CHECK: ![[ORIGMIB3]] = !{![[ORIGMIBSTACK3:[0-9]+]], !"notcold"} +; CHECK: ![[ORIGMIBSTACK3]] = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -4805294506621015872} +; CHECK: ![[ORIGMIB4]] = !{![[ORIGMIBSTACK4:[0-9]+]], !"cold"} +; CHECK: ![[ORIGMIBSTACK4]] = !{i64 -2458008693472584243, i64 -8079659623765193173, i64 -972865200055133905} +; CHECK: ![[NEWMEMPROF]] = !{![[ORIGMIB3:[0-9]+]], ![[ORIGMIB4:[0-9]+]]} ; CHECK: ![[NEWCALLSITE]] = !{i64 -2458008693472584243, i64 -8079659623765193173} !52 = !{i64 -2458008693472584243} !53 = !DILocation(line: 5, column: 3, scope: !39) -- 2.7.4