-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[WIP] Handle guard insertion in callbacks to OpenMP runtime functions. #164655
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
Draft
abidh
wants to merge
12
commits into
llvm:users/skatrak/flang-generic-11-shared-mem-checks
Choose a base branch
from
abidh:spdm_callbacks
base: users/skatrak/flang-generic-11-shared-mem-checks
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
[WIP] Handle guard insertion in callbacks to OpenMP runtime functions. #164655
abidh
wants to merge
12
commits into
llvm:users/skatrak/flang-generic-11-shared-mem-checks
from
abidh:spdm_callbacks
+246
−16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp -- llvm/lib/Transforms/IPO/OpenMPOpt.cpp
View the diff from clang-format here.diff --git a/llvm/lib/Transforms/IPO/OpenMPOpt.cpp b/llvm/lib/Transforms/IPO/OpenMPOpt.cpp
index 4181c794c..6b888a4ca 100644
--- a/llvm/lib/Transforms/IPO/OpenMPOpt.cpp
+++ b/llvm/lib/Transforms/IPO/OpenMPOpt.cpp
@@ -5001,28 +5001,28 @@ struct AAKernelInfoCallSite : AAKernelInfo {
// won't be any change so we indicate a fixpoint.
indicateOptimisticFixpoint();
}
- // If the callee is known and can be used in IPO, we will update the
- // state based on the callee state in updateImpl.
- return;
- }
- // Check if we have multiple possible callees. This usually indicates an
- // indirect call where we don't know the target, requiring a pessimistic
- // fixpoint. However, for callback functions, multiple edges are expected:
- // one to the runtime function and edges through callback parameters. These
- // are analyzable, so we exclude them from the pessimistic check.
- if (NumCallees > 1 && !Callee->hasMetadata(LLVMContext::MD_callback)) {
- LLVM_DEBUG(dbgs() << "[OpenMPOpt] Multiple callees found, forcing "
- "pessimistic fixpoint\n");
- indicatePessimisticFixpoint();
- return;
- }
- LLVM_DEBUG({
- if (NumCallees > 1 && Callee->hasMetadata(LLVMContext::MD_callback)) {
- dbgs() << "[OpenMPOpt] Allowing multiple callees for callback "
- "function: "
- << Callee->getName() << "\n";
+ // If the callee is known and can be used in IPO, we will update the
+ // state based on the callee state in updateImpl.
+ return;
}
- });
+ // Check if we have multiple possible callees. This usually indicates an
+ // indirect call where we don't know the target, requiring a pessimistic
+ // fixpoint. However, for callback functions, multiple edges are expected:
+ // one to the runtime function and edges through callback parameters.
+ // These are analyzable, so we exclude them from the pessimistic check.
+ if (NumCallees > 1 && !Callee->hasMetadata(LLVMContext::MD_callback)) {
+ LLVM_DEBUG(dbgs() << "[OpenMPOpt] Multiple callees found, forcing "
+ "pessimistic fixpoint\n");
+ indicatePessimisticFixpoint();
+ return;
+ }
+ LLVM_DEBUG({
+ if (NumCallees > 1 && Callee->hasMetadata(LLVMContext::MD_callback)) {
+ dbgs() << "[OpenMPOpt] Allowing multiple callees for callback "
+ "function: "
+ << Callee->getName() << "\n";
+ }
+ });
RuntimeFunction RF = It->getSecond();
switch (RF) {
@@ -5185,28 +5185,28 @@ struct AAKernelInfoCallSite : AAKernelInfo {
A.getAAFor<AAKernelInfo>(*this, FnPos, DepClassTy::REQUIRED);
if (!FnAA)
return indicatePessimisticFixpoint();
- if (getState() == FnAA->getState())
- return ChangeStatus::UNCHANGED;
- getState() = FnAA->getState();
- return ChangeStatus::CHANGED;
- }
- // Check if we have multiple possible callees. This usually indicates an
- // indirect call where we don't know the target, requiring a pessimistic
- // fixpoint. However, for callback functions, multiple edges are expected:
- // one to the runtime function and edges through callback parameters. These
- // are analyzable, so we exclude them from the pessimistic check.
- if (NumCallees > 1 && !F->hasMetadata(LLVMContext::MD_callback)) {
- LLVM_DEBUG(dbgs() << "[OpenMPOpt] Multiple callees in update, forcing "
- "pessimistic fixpoint\n");
- return indicatePessimisticFixpoint();
- }
- LLVM_DEBUG({
- if (NumCallees > 1 && F->hasMetadata(LLVMContext::MD_callback)) {
- dbgs() << "[OpenMPOpt] Allowing multiple callees for callback "
- "function in update: "
- << F->getName() << "\n";
+ if (getState() == FnAA->getState())
+ return ChangeStatus::UNCHANGED;
+ getState() = FnAA->getState();
+ return ChangeStatus::CHANGED;
}
- });
+ // Check if we have multiple possible callees. This usually indicates an
+ // indirect call where we don't know the target, requiring a pessimistic
+ // fixpoint. However, for callback functions, multiple edges are expected:
+ // one to the runtime function and edges through callback parameters.
+ // These are analyzable, so we exclude them from the pessimistic check.
+ if (NumCallees > 1 && !F->hasMetadata(LLVMContext::MD_callback)) {
+ LLVM_DEBUG(dbgs() << "[OpenMPOpt] Multiple callees in update, forcing "
+ "pessimistic fixpoint\n");
+ return indicatePessimisticFixpoint();
+ }
+ LLVM_DEBUG({
+ if (NumCallees > 1 && F->hasMetadata(LLVMContext::MD_callback)) {
+ dbgs() << "[OpenMPOpt] Allowing multiple callees for callback "
+ "function in update: "
+ << F->getName() << "\n";
+ }
+ });
CallBase &CB = cast<CallBase>(getAssociatedValue());
if (It->getSecond() == OMPRTL___kmpc_parallel_51) {
|
Also set metadata in case it was not set (can happen with linked library functions).
Get the information from the def file instead.
This reverts commit afb76ff.
…ks." This reverts commit 56037a6.
This commit improves the robustness and maintainability of callback
function analysis in OpenMPOpt by adding:
1. Defensive check for function declarations:
- Skip analysis of declaration-only callbacks that cannot be analyzed
interprocedurally
- Prevents attempting to create AAKernelInfo for external functions
without definitions
2. Explanatory comments for multiple callees handling:
- Document why callback functions are excluded from pessimistic
fixpoint when NumCallees > 1
- Clarify that callbacks create expected multiple edges (to runtime
function and through callback parameters) which are analyzable
3. Debug output for callback analysis:
- Log when analyzing a callback function
- Log when skipping declaration-only functions
- Log decisions about allowing/forcing pessimistic fixpoints
- Warn in debug builds when callback resolution fails
4. Verification in debug builds:
- Assert expectations about callback resolution
- Help catch logic errors early during development
These improvements make the code more robust against edge cases (like
linked library functions) and easier to debug when callback analysis
doesn't work as expected.
No functional changes for well-formed input - only improved error
handling and diagnostics.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.