[FunctionAttrs] Don't bail out on unknown calls #150958
Merged
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
When inferring attributes, we should not bail out early on unknown calls (such as virtual calls), as we may still have call-site attributes that can be used for inference. Fixes llvm#150817.
|
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesWhen inferring attributes, we should not bail out early on unknown calls (such as virtual calls), as we may still have call-site attributes that can be used for inference. This drops the HasUnknownCall flag and adjusts the norecurse implementation to check call attributes (all other inference already did that). Fixes #150817. Full diff: https://github.com/llvm/llvm-project/pull/150958.diff 5 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index f43202eea6306..7fde4605e5686 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -1863,7 +1863,6 @@ void AttributeInferer::run(const SCCNodeSet &SCCNodes,
struct SCCNodesResult {
SCCNodeSet SCCNodes;
- bool HasUnknownCall;
};
} // end anonymous namespace
@@ -2087,11 +2086,13 @@ static void addNoRecurseAttrs(const SCCNodeSet &SCCNodes,
for (auto &BB : *F)
for (auto &I : BB.instructionsWithoutDebug())
if (auto *CB = dyn_cast<CallBase>(&I)) {
+ if (CB->hasFnAttr(Attribute::NoRecurse))
+ continue;
+
Function *Callee = CB->getCalledFunction();
if (!Callee || Callee == F ||
- (!Callee->doesNotRecurse() &&
- !(Callee->isDeclaration() &&
- Callee->hasFnAttribute(Attribute::NoCallback))))
+ !(Callee->isDeclaration() &&
+ Callee->hasFnAttribute(Attribute::NoCallback)))
// Function calls a potentially recursive function.
return;
}
@@ -2227,29 +2228,13 @@ static void addWillReturn(const SCCNodeSet &SCCNodes,
static SCCNodesResult createSCCNodeSet(ArrayRef<Function *> Functions) {
SCCNodesResult Res;
- Res.HasUnknownCall = false;
for (Function *F : Functions) {
if (!F || F->hasOptNone() || F->hasFnAttribute(Attribute::Naked) ||
F->isPresplitCoroutine()) {
- // Treat any function we're trying not to optimize as if it were an
- // indirect call and omit it from the node set used below.
- Res.HasUnknownCall = true;
+ // Omit any functions we're trying not to optimize from the set.
continue;
}
- // Track whether any functions in this SCC have an unknown call edge.
- // Note: if this is ever a performance hit, we can common it with
- // subsequent routines which also do scans over the instructions of the
- // function.
- if (!Res.HasUnknownCall) {
- for (Instruction &I : instructions(*F)) {
- if (auto *CB = dyn_cast<CallBase>(&I)) {
- if (!CB->getCalledFunction()) {
- Res.HasUnknownCall = true;
- break;
- }
- }
- }
- }
+
Res.SCCNodes.insert(F);
}
return Res;
@@ -2282,15 +2267,10 @@ deriveAttrsInPostOrder(ArrayRef<Function *> Functions, AARGetterT &&AARGetter,
addColdAttrs(Nodes.SCCNodes, Changed);
addWillReturn(Nodes.SCCNodes, Changed);
addNoUndefAttrs(Nodes.SCCNodes, Changed);
-
- // If we have no external nodes participating in the SCC, we can deduce some
- // more precise attributes as well.
- if (!Nodes.HasUnknownCall) {
- addNoAliasAttrs(Nodes.SCCNodes, Changed);
- addNonNullAttrs(Nodes.SCCNodes, Changed);
- inferAttrsFromFunctionBodies(Nodes.SCCNodes, Changed);
- addNoRecurseAttrs(Nodes.SCCNodes, Changed);
- }
+ addNoAliasAttrs(Nodes.SCCNodes, Changed);
+ addNonNullAttrs(Nodes.SCCNodes, Changed);
+ inferAttrsFromFunctionBodies(Nodes.SCCNodes, Changed);
+ addNoRecurseAttrs(Nodes.SCCNodes, Changed);
// Finally, infer the maximal set of attributes from the ones we've inferred
// above. This is handling the cases where one attribute on a signature
diff --git a/llvm/test/Transforms/FunctionAttrs/noalias.ll b/llvm/test/Transforms/FunctionAttrs/noalias.ll
index 8beb6fe78f852..de8bd9eade621 100644
--- a/llvm/test/Transforms/FunctionAttrs/noalias.ll
+++ b/llvm/test/Transforms/FunctionAttrs/noalias.ll
@@ -235,7 +235,7 @@ define ptr @return_unknown_call(ptr %fn) {
}
define ptr @return_unknown_noalias_call(ptr %fn) {
-; CHECK-LABEL: define ptr @return_unknown_noalias_call(
+; CHECK-LABEL: define noalias ptr @return_unknown_noalias_call(
; CHECK-SAME: ptr readonly captures(none) [[FN:%.*]]) {
; CHECK-NEXT: [[A:%.*]] = call noalias ptr [[FN]]()
; CHECK-NEXT: ret ptr [[A]]
diff --git a/llvm/test/Transforms/FunctionAttrs/nonnull.ll b/llvm/test/Transforms/FunctionAttrs/nonnull.ll
index 9b17ded3bdb26..8df242fb023af 100644
--- a/llvm/test/Transforms/FunctionAttrs/nonnull.ll
+++ b/llvm/test/Transforms/FunctionAttrs/nonnull.ll
@@ -1412,7 +1412,7 @@ define ptr @unknown_func(ptr %fn) {
}
define ptr @unknown_nonnull_func(ptr %fn) {
-; FNATTRS-LABEL: define ptr @unknown_nonnull_func(
+; FNATTRS-LABEL: define nonnull ptr @unknown_nonnull_func(
; FNATTRS-SAME: ptr readonly captures(none) [[FN:%.*]]) {
; FNATTRS-NEXT: [[RES:%.*]] = call nonnull ptr [[FN]]()
; FNATTRS-NEXT: ret ptr [[RES]]
diff --git a/llvm/test/Transforms/FunctionAttrs/norecurse.ll b/llvm/test/Transforms/FunctionAttrs/norecurse.ll
index 5cb8ac05847aa..a9437848150da 100644
--- a/llvm/test/Transforms/FunctionAttrs/norecurse.ll
+++ b/llvm/test/Transforms/FunctionAttrs/norecurse.ll
@@ -258,9 +258,10 @@ define void @unknown_call(ptr %fn) {
}
define void @unknown_norecurse_call(ptr %fn) {
+; FNATTRS: Function Attrs: norecurse
; FNATTRS-LABEL: define {{[^@]+}}@unknown_norecurse_call
-; FNATTRS-SAME: (ptr readonly captures(none) [[FN:%.*]]) {
-; FNATTRS-NEXT: call void [[FN]]() #[[ATTR7:[0-9]+]]
+; FNATTRS-SAME: (ptr readonly captures(none) [[FN:%.*]]) #[[ATTR7:[0-9]+]] {
+; FNATTRS-NEXT: call void [[FN]]() #[[ATTR7]]
; FNATTRS-NEXT: ret void
;
; ATTRIBUTOR-LABEL: define {{[^@]+}}@unknown_norecurse_call
diff --git a/llvm/test/Transforms/FunctionAttrs/nounwind.ll b/llvm/test/Transforms/FunctionAttrs/nounwind.ll
index a64d9a6504256..076a7df2781ce 100644
--- a/llvm/test/Transforms/FunctionAttrs/nounwind.ll
+++ b/llvm/test/Transforms/FunctionAttrs/nounwind.ll
@@ -418,9 +418,10 @@ define void @unknown_call(ptr %fn) {
}
define void @unknown_nounwind_call(ptr %fn) {
+; FNATTRS: Function Attrs: nounwind
; FNATTRS-LABEL: define {{[^@]+}}@unknown_nounwind_call
-; FNATTRS-SAME: (ptr readonly captures(none) [[FN:%.*]]) {
-; FNATTRS-NEXT: call void [[FN]]() #[[ATTR2:[0-9]+]]
+; FNATTRS-SAME: (ptr readonly captures(none) [[FN:%.*]]) #[[ATTR2:[0-9]+]] {
+; FNATTRS-NEXT: call void [[FN]]() #[[ATTR2]]
; FNATTRS-NEXT: ret void
;
; ATTRIBUTOR: Function Attrs: nounwind
|
antoniofrighetto
approved these changes
Jul 28, 2025
| for (auto &I : BB.instructionsWithoutDebug()) | ||
| if (auto *CB = dyn_cast<CallBase>(&I)) { | ||
| if (CB->hasFnAttr(Attribute::NoRecurse)) | ||
| continue; |
There was a problem hiding this comment.
Is this actually legal? norecurse on a call just means that the callee doesn't call itself; it doesn't necessarily mean it doesn't call the caller. For example:
void foo(), bar(int);
void foo() { bar(0); }
void bar(int b) { if (b) foo(); }
"foo" is norecurse, but bar isn't.
There was a problem hiding this comment.
This is a general issue with norecurse inference, independently of whether the attribute is on the call-site or the declaration. See the discussion on #139943 (comment). I expect we'll change the definition of norecurse to justify the correctness of the existing inference (which would also make this change valid).
But I'm happy to drop this change until that has happened.
There was a problem hiding this comment.
Oh, didn't realize there was already an active discussion. If the current norecurse inference is self-consistent, that fine for now.
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.
When inferring attributes, we should not bail out early on unknown calls (such as virtual calls), as we may still have call-site attributes that can be used for inference.
This drops the HasUnknownCall flag and adjusts the norecurse implementation to check call attributes (all other inference already did that).
Fixes #150817.