Skip to content

[MemProf] Avoid assertion checking loop under NDEBUG (NFC) #138985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 8, 2025

Conversation

teresajohnson
Copy link
Contributor

Guard a loop that only exists to do assertion checking of stack ids on
memprof metadata so that it isn't compiled and executed under NDEBUG.
This is similar to how callsite metadata stack id verification is
guarded further below.

Guard a loop that only exists to do assertion checking of stack ids on
memprof metadata so that it isn't compiled and executed under NDEBUG.
This is similar to how callsite metadata stack id verification is
guarded further below.
@llvmbot
Copy link
Member

llvmbot commented May 7, 2025

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Teresa Johnson (teresajohnson)

Changes

Guard a loop that only exists to do assertion checking of stack ids on
memprof metadata so that it isn't compiled and executed under NDEBUG.
This is similar to how callsite metadata stack id verification is
guarded further below.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp (+2)
diff --git a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
index ec158afb76519..567f0acadbd2d 100644
--- a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
+++ b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
@@ -5242,6 +5242,7 @@ bool MemProfContextDisambiguation::applyImport(Module &M) {
           assert(AI != FS->allocs().end());
           auto &AllocNode = *(AI++);
 
+#ifndef NDEBUG
           // Sanity check that the MIB stack ids match between the summary and
           // instruction metadata.
           auto MIBIter = AllocNode.MIBs.begin();
@@ -5276,6 +5277,7 @@ bool MemProfContextDisambiguation::applyImport(Module &M) {
             }
             MIBIter++;
           }
+#endif
 
           // Perform cloning if not yet done.
           CloneFuncIfNeeded(/*NumClones=*/AllocNode.Versions.size());

@@ -5242,6 +5242,7 @@ bool MemProfContextDisambiguation::applyImport(Module &M) {
assert(AI != FS->allocs().end());
auto &AllocNode = *(AI++);

#ifndef NDEBUG
// Sanity check that the MIB stack ids match between the summary and
// instruction metadata.
auto MIBIter = AllocNode.MIBs.begin();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we refactor this out to a static helper? I think it should be straightforward since the only live ins for this region are AllocNode and MemProfMD as far as I can tell.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. There were a couple of other live in variables. I also updated one of the methods to take a const to enable using const for the new helper.

@llvmbot llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label May 8, 2025
Copy link
Contributor

@snehasish snehasish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -5050,6 +5050,45 @@ bool MemProfContextDisambiguation::initializeIndirectCallPromotionInfo(
return true;
}

#ifndef NDEBUG
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can drop the ifndef around the function definition in favour of a LLVM_ATTRIBUTE_UNUSED function attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could, but then I need to put that attribute back on the StackIdIndexIter def in this code that I had removed it from. My sense is that the #ifdef is simpler overall, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

@teresajohnson teresajohnson merged commit 7348d7e into llvm:main May 8, 2025
8 of 11 checks passed
petrhosek pushed a commit to petrhosek/llvm-project that referenced this pull request May 8, 2025
Guard a loop that only exists to do assertion checking of stack ids on
memprof metadata so that it isn't compiled and executed under NDEBUG.
This is similar to how callsite metadata stack id verification is
guarded further below.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants