[MemProf] Fix when CallStackTrie has a single chain to leaf with multi alloc type #79433

lifengxiang1025 · 2024-01-25T10:17:34Z

Fix one corner case when CallStackTrie has a single chain to leaf with multi alloc type. This will cause stackIds in function summary is empty.

llvmbot · 2024-01-25T10:18:05Z

@llvm/pr-subscribers-pgo
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: None (lifengxiang1025)

Changes

Fix one corner case when CallStackTrie is a single chain with multi alloc type. This will cause stackIds in function summary is empty.

Full diff: https://github.com/llvm/llvm-project/pull/79433.diff

1 Files Affected:

(modified) llvm/lib/Analysis/MemoryProfileInfo.cpp (+8-6)

diff --git a/llvm/lib/Analysis/MemoryProfileInfo.cpp b/llvm/lib/Analysis/MemoryProfileInfo.cpp
index 7fbcffc6489dcfd..7fbbc280348c079 100644
--- a/llvm/lib/Analysis/MemoryProfileInfo.cpp
+++ b/llvm/lib/Analysis/MemoryProfileInfo.cpp
@@ -244,12 +244,14 @@ bool CallStackTrie::buildAndAttachMIBMetadata(CallBase *CI) {
   MIBCallStack.push_back(AllocStackId);
   std::vector<Metadata *> MIBNodes;
   assert(!Alloc->Callers.empty() && "addCallStack has not been called yet");
-  buildMIBNodes(Alloc, Ctx, MIBCallStack, MIBNodes,
-                /*CalleeHasAmbiguousCallerContext=*/true);
-  assert(MIBCallStack.size() == 1 &&
-         "Should only be left with Alloc's location in stack");
-  CI->setMetadata(LLVMContext::MD_memprof, MDNode::get(Ctx, MIBNodes));
-  return true;
+  if (buildMIBNodes(Alloc, Ctx, MIBCallStack, MIBNodes,
+                    Alloc->Callers.size() > 1)) {
+    assert(MIBCallStack.size() == 1 &&
+           "Should only be left with Alloc's location in stack");
+    CI->setMetadata(LLVMContext::MD_memprof, MDNode::get(Ctx, MIBNodes));
+    return true;
+  }
+  return false;
 }
 
 template <>

lifengxiang1025 · 2024-01-25T10:22:44Z

I met one coredump when I try memprof in our benchmark. After debugging, I found there exists three same callstack with different alloc type when build one AllocTrie. I don't know why there are same callstack. So I can't write test case now.

lifengxiang1025 · 2024-01-26T13:48:18Z

I don't know why there are same callstack

I think the reason is that the callstack's raw address is not same but the correspond function name, line offset and column are same. This happens when compiler unrolls the loop or does other similar optimization.
So I think we may use Frame stack not address stack as key to identify unique AllocSites in llvm-profdata.
Problem demo:

Stack for id 2:
    #0 0x2aeb0d in operator new[](unsigned long) /data00/lifengxiang.1025/lfx/llvm-project/compiler-rt/lib/memprof/memprof_new_delete.cpp:52:3
    #1 0x2afe5a in foo() /data00/lifengxiang.1025/test/memprof1/b.cpp:6:16
    #2 0x2afdf4 in main /data00/lifengxiang.1025/test/memprof1/a.cpp:4:5
    #3 0x7ffbdc9d609a in __libc_start_main /build/glibc-vjB4T1/glibc-2.28/csu/../csu/libc-start.c:308:16

Stack for id 3:
    #0 0x2aeb0d in operator new[](unsigned long) /data00/lifengxiang.1025/lfx/llvm-project/compiler-rt/lib/memprof/memprof_new_delete.cpp:52:3
    #1 0x2afe79 in foo() /data00/lifengxiang.1025/test/memprof1/b.cpp:6:16
    #2 0x2afdf4 in main /data00/lifengxiang.1025/test/memprof1/a.cpp:4:5
    #3 0x7ffbdc9d609a in __libc_start_main /build/glibc-vjB4T1/glibc-2.28/csu/../csu/libc-start.c:308:16

lifengxiang1025 · 2024-01-26T15:38:26Z

Add test case. Please review.

teresajohnson · 2024-01-31T18:16:00Z

Sorry for the slow reply, I have been mostly OOO for the past week.

I think the reason is that the callstack's raw address is not same but the correspond function name, line offset and column are same. This happens when compiler unrolls the loop or does other similar optimization. So I think we may use Frame stack not address stack as key to identify unique AllocSites in llvm-profdata. Problem demo:

Interesting, I do agree that llvm-profdata should merge these. @snehasish is the person most familiar with the llvm-profdata code, but he is currently OOO as well.

Stack for id 2:
    #0 0x2aeb0d in operator new[](unsigned long) /data00/lifengxiang.1025/lfx/llvm-project/compiler-rt/lib/memprof/memprof_new_delete.cpp:52:3
    #1 0x2afe5a in foo() /data00/lifengxiang.1025/test/memprof1/b.cpp:6:16
    #2 0x2afdf4 in main /data00/lifengxiang.1025/test/memprof1/a.cpp:4:5
    #3 0x7ffbdc9d609a in __libc_start_main /build/glibc-vjB4T1/glibc-2.28/csu/../csu/libc-start.c:308:16

Stack for id 3:
    #0 0x2aeb0d in operator new[](unsigned long) /data00/lifengxiang.1025/lfx/llvm-project/compiler-rt/lib/memprof/memprof_new_delete.cpp:52:3
    #1 0x2afe79 in foo() /data00/lifengxiang.1025/test/memprof1/b.cpp:6:16
    #2 0x2afdf4 in main /data00/lifengxiang.1025/test/memprof1/a.cpp:4:5
    #3 0x7ffbdc9d609a in __libc_start_main /build/glibc-vjB4T1/glibc-2.28/csu/../csu/libc-start.c:308:16

lifengxiang1025 · 2024-02-01T02:30:14Z

Interesting, I do agree that llvm-profdata should merge these. @snehasish is the person most familiar with the llvm-profdata code, but he is currently OOO as well.

@teresajohnson Thanks for reply. How about merging this PR to fix this corner case firstly? Or you want to modify llvm-profdata directly?

teresajohnson · 2024-02-01T05:18:20Z

Interesting, I do agree that llvm-profdata should merge these. @snehasish is the person most familiar with the llvm-profdata code, but he is currently OOO as well.

@teresajohnson Thanks for reply. How about merging this PR to fix this corner case firstly? Or you want to modify llvm-profdata directly?

I think we should put in a fix for it now until it can be fixed in llvm-profdata. I had a suggestion on the PR.

lifengxiang1025 · 2024-02-01T07:24:14Z

I had a suggestion on the PR.

Sorry, I don't quite understand. What is your suggestion?

teresajohnson

Somehow when I left my earlier comment it didn't actually send the inline review comments.

llvm/lib/Analysis/MemoryProfileInfo.cpp

teresajohnson · 2024-02-01T14:14:42Z

I had a suggestion on the PR.

Sorry, I don't quite understand. What is your suggestion?

Woops, still getting used to the PR interface. The comment was left as pending and should have been sent now.

teresajohnson

lgtm, one minor fix below.

teresajohnson · 2024-02-02T13:14:26Z

llvm/lib/Analysis/MemoryProfileInfo.cpp

-         "Should only be left with Alloc's location in stack");
-  CI->setMetadata(LLVMContext::MD_memprof, MDNode::get(Ctx, MIBNodes));
-  return true;
+  if (buildMIBNodes(Alloc, Ctx, MIBCallStack, MIBNodes, false)) {


Document const parameter (i.e. like it currently is at head but with false instead of true).

…i alloc type (llvm#79433) Fix one corner case when `CallStackTrie` has a single chain to leaf with multi alloc type. This will cause stackIds in function summary is empty.

llvmbot added the llvm:analysis label Jan 25, 2024

lifengxiang1025 requested a review from teresajohnson January 25, 2024 10:18

lifengxiang1025 self-assigned this Jan 25, 2024

lifengxiang1025 force-pushed the lfx.fix2 branch from 311548f to 9884c20 Compare January 25, 2024 12:15

lifengxiang1025 requested a review from snehasish January 26, 2024 02:49

[MemProf] Fix when CallStackTrie is a single chain with multi alloc type

962697e

lifengxiang1025 force-pushed the lfx.fix2 branch from 9884c20 to 962697e Compare January 26, 2024 15:36

llvmbot added PGO Profile Guided Optimizations llvm:transforms labels Jan 26, 2024

lifengxiang1025 removed their assignment Jan 27, 2024

teresajohnson reviewed Feb 1, 2024

View reviewed changes

llvm/lib/Analysis/MemoryProfileInfo.cpp Outdated Show resolved Hide resolved

fix

6e3f71b

teresajohnson approved these changes Feb 2, 2024

View reviewed changes

lifengxiang1025 changed the title ~~[MemProf] Fix when CallStackTrie is a single chain with multi alloc type~~ [MemProf] Fix when CallStackTrie has a single chain to leaf with multi alloc type Feb 2, 2024

update comment

4e1a124

lifengxiang1025 merged commit 1e7d587 into llvm:main Feb 2, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MemProf] Fix when CallStackTrie has a single chain to leaf with multi alloc type #79433

[MemProf] Fix when CallStackTrie has a single chain to leaf with multi alloc type #79433

lifengxiang1025 commented Jan 25, 2024 •

edited

llvmbot commented Jan 25, 2024 •

edited

lifengxiang1025 commented Jan 25, 2024 •

edited

lifengxiang1025 commented Jan 26, 2024 •

edited

lifengxiang1025 commented Jan 26, 2024

teresajohnson commented Jan 31, 2024

lifengxiang1025 commented Feb 1, 2024

teresajohnson commented Feb 1, 2024

lifengxiang1025 commented Feb 1, 2024 •

edited

teresajohnson left a comment

teresajohnson commented Feb 1, 2024

teresajohnson left a comment

teresajohnson Feb 2, 2024

[MemProf] Fix when CallStackTrie has a single chain to leaf with multi alloc type #79433

[MemProf] Fix when CallStackTrie has a single chain to leaf with multi alloc type #79433

Conversation

lifengxiang1025 commented Jan 25, 2024 • edited

llvmbot commented Jan 25, 2024 • edited

lifengxiang1025 commented Jan 25, 2024 • edited

lifengxiang1025 commented Jan 26, 2024 • edited

lifengxiang1025 commented Jan 26, 2024

teresajohnson commented Jan 31, 2024

lifengxiang1025 commented Feb 1, 2024

teresajohnson commented Feb 1, 2024

lifengxiang1025 commented Feb 1, 2024 • edited

teresajohnson left a comment

Choose a reason for hiding this comment

teresajohnson commented Feb 1, 2024

teresajohnson left a comment

Choose a reason for hiding this comment

teresajohnson Feb 2, 2024

Choose a reason for hiding this comment

lifengxiang1025 commented Jan 25, 2024 •

edited

llvmbot commented Jan 25, 2024 •

edited

lifengxiang1025 commented Jan 25, 2024 •

edited

lifengxiang1025 commented Jan 26, 2024 •

edited

lifengxiang1025 commented Feb 1, 2024 •

edited