[MemProf] Fix assert when exists direct recursion

[lifengxiang1025]

Fix assert in MemProfContextDisambiguation::applyImport when exists direct recursion.

[llvmbot]

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

@llvm/pr-subscribers-llvm-analysis

Author: None (lifengxiang1025)

Changes

I met one assert in MemProfContextDisambiguation::applyImport because metadata doesn't match summary. The reason is function summary skip direct recursion but metadata donen't.

---

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

1 Files Affected:

• (modified) llvm/lib/Analysis/ModuleSummaryAnalysis.cpp (+1-5)

diff --git a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
index 1f15e94783240a..7e09192c5f1d51 100644
--- a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
+++ b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
@@ -499,11 +499,7 @@ static void computeFunctionSummary(
                    StackContext.beginAfterSharedPrefix(InstCallsite);
                ContextIter != StackContext.end(); ++ContextIter) {
             unsigned StackIdIdx = Index.addOrGetStackIdIndex(*ContextIter);
-            // If this is a direct recursion, simply skip the duplicate
-            // entries. If this is mutual recursion, handling is left to
-            // the LTO link analysis client.
-            if (StackIdIndices.empty() || StackIdIndices.back() != StackIdIdx)
-              StackIdIndices.push_back(StackIdIdx);
+            StackIdIndices.push_back(StackIdIdx);
           }
           MIBs.push_back(
               MIBInfo(getMIBAllocType(MIBMD), std::move(StackIdIndices)));

[lifengxiang1025]

I'm not sure why we treat direct recursion specially. Why don't we treat all recursion in the same way？For example:
foo->foo->foo
foo->bar->foo->bar

[teresajohnson]

I'm not sure why we treat direct recursion specially. Why don't we treat all recursion in the same way？For example: foo->foo->foo foo->bar->foo->bar

At the time I was thinking that it is a little easier to get the direct recursion case right in some simple cases, but I'm not sure this is the right way to handle it as I think through it again now. We should probably treat them the same, which I think this change will essentially do (see my reply on discourse, I think we should be handling recursion detected in the thin link conservatively).

Can you attach a test case?

[lifengxiang1025]

Can you attach a test case?

Add test case. Please review.

[teresajohnson]

See my reply on the Discourse thread just now here: https://discourse.llvm.org/t/rfc-ir-metadata-format-for-memprof/59165/27?u=teresajohnson

I remembered the reason I was handling direct recursion differently. I wonder if we can fix the assert that was firing in this case (and check that we do get the expected cloning)?

[lifengxiang1025]

I remembered the reason I was handling direct recursion differently. I wonder if we can fix the assert that was firing in this case (and check that we do get the expected cloning)?

Thanks! I'll fix the assert and add test case soon.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[teresajohnson]

lgtm thanks!

[teresajohnson]

I ran into an assert in the operator* method because ContextIterBegin was already at StackContext.end() (after skipping the shared prefix above). I think this should be:

uint64_t LastStackContextId = (ContextIter != StackContext.end() && *ContextIterBegin == 0) ? 1 : 0;

Can you send a fix?

[lifengxiang1025]

Ok, OOO, fix later.

[lifengxiang1025]

@teresajohnson I think ContextIter should't at StackContext.end() because we already have assert assert(AI != FS->allocs().end());. How about adding one more assert assert(ContextIterBegin != StackContext.end());? #81004

[teresajohnson]

The allocs list on the FS is different than the list of context ids on a given MIB though, so not sure how that assert is related?

The assert you propose will fire in the case that I saw, this is the reason we are getting the assert in operator*. Since this is breaking our memprof builds I have a fix and a testcase that I will send for review shortly. I'm not exactly sure why we are seeing this case, I assume it happened during inlining but in that case we should have ended up replacing the memprof metadata with an attribute. I have a FIXME in the test to investigate this. But at this point I want to unblock our builds.

[teresajohnson]

#81008

[lifengxiang1025]

But at this point I want to unblock our builds.

I think ContextIter should't at StackContext.end(). So maybe add one assert is more reasonable. It's ok to do this to unblock your builds. Thanks for reply.