Skip to content
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

[MemProf] Fix an incorrect iterator increment #123438

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

teresajohnson
Copy link
Contributor

We pass in a pointer to an Edge iterator to
moveEdgeToExistingCalleeClone, so that it can be correctly updated when
we remove edges during an edge iteration. We were not dereferencing this
pointer in one case, meaning we would increment the pointer and not the
iterator as intended.

This did not cause any issues, as it turns out that we would simply skip
the edge on the next iteration as it was already appropriately handled.
While in theory this incurred some extra compilation time, in practice
for a large application the effect was not significant. I confirmed that
there was no effect to any cloning from the fix.

I plan to send a follow up change to avoid the need to pass in an
iterator at all and simplify / consolidate the handling in the caller,
but want to fix this in case something requires a revert of the follow
on fix.

We pass in a pointer to an Edge iterator to
moveEdgeToExistingCalleeClone, so that it can be correctly updated when
we remove edges during an edge iteration. We were not dereferencing this
pointer in one case, meaning we would increment the pointer and not the
iterator as intended.

This did not cause any issues, as it turns out that we would simply skip
the edge on the next iteration as it was already appropriately handled.
While in theory this incurred some extra compilation time, in practice
for a large application the effect was not significant. I confirmed that
there was no effect to any cloning from the fix.

I plan to send a follow up change to avoid the need to pass in an
iterator at all and simplify / consolidate the handling in the caller,
but want to fix this in case something requires a revert of the follow
on fix.
@llvmbot
Copy link
Member

llvmbot commented Jan 18, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Teresa Johnson (teresajohnson)

Changes

We pass in a pointer to an Edge iterator to
moveEdgeToExistingCalleeClone, so that it can be correctly updated when
we remove edges during an edge iteration. We were not dereferencing this
pointer in one case, meaning we would increment the pointer and not the
iterator as intended.

This did not cause any issues, as it turns out that we would simply skip
the edge on the next iteration as it was already appropriately handled.
While in theory this incurred some extra compilation time, in practice
for a large application the effect was not significant. I confirmed that
there was no effect to any cloning from the fix.

I plan to send a follow up change to avoid the need to pass in an
iterator at all and simplify / consolidate the handling in the caller,
but want to fix this in case something requires a revert of the follow
on fix.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp (+1-1)
diff --git a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
index 61a8f4a448bbd7..6fdf78373ba598 100644
--- a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
+++ b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
@@ -3102,7 +3102,7 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::
   } else {
     // Only moving a subset of Edge's ids.
     if (CallerEdgeI)
-      ++CallerEdgeI;
+      ++(*CallerEdgeI);
     // Compute the alloc type of the subset of ids being moved.
     auto CallerEdgeAllocType = computeAllocType(ContextIdsToMove);
     if (ExistingEdgeToNewCallee) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants