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

[ArgPromotion] Consider InvokeInst in Caller alias analysis #110335

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

hazzlim
Copy link
Contributor

@hazzlim hazzlim commented Sep 27, 2024

Check that all users of a Function are CallBase rather than CallInst when performing alias analysis using actual arguments in the calling function, as this check is also valid for Invoke instructions.

This allows replacing the existing check with an assert, as the Function only being used by CallBase derived instructions is a precondition of the transform.

This addresses post-commit review on #106216.

Check that all users of a Function are CallBase rather than CallInst
when performing alias analysis using actual arguments in the calling
function, as this check is also valid for Invoke instructions.

This allows replacing the existing check with an assert, as the Function
only being used by CallBase derived instructions is a precondition of
the transform.

This addresses post-commit review on llvm#106216.
@llvmbot
Copy link
Member

llvmbot commented Sep 27, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Hari Limaye (hazzlim)

Changes

Check that all users of a Function are CallBase rather than CallInst when performing alias analysis using actual arguments in the calling function, as this check is also valid for Invoke instructions.

This allows replacing the existing check with an assert, as the Function only being used by CallBase derived instructions is a precondition of the transform.

This addresses post-commit review on #106216.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/ArgumentPromotion.cpp (+2-4)
diff --git a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
index 90e8c39e5a90df..590362bdcad2cd 100644
--- a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
+++ b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
@@ -491,10 +491,8 @@ static bool isArgUnmodifiedByAllCalls(Argument *Arg,
                                       FunctionAnalysisManager &FAM) {
   for (User *U : Arg->getParent()->users()) {
 
-    // Bail if we find an unexpected (non CallInst) use of the function.
-    auto *Call = dyn_cast<CallInst>(U);
-    if (!Call)
-      return false;
+    assert(isa<CallBase>(U) && "Expected all users of Function to be CallBase");
+    CallBase *Call = cast<CallBase>(U);
 
     MemoryLocation Loc =
         MemoryLocation::getForArgument(Call, Arg->getArgNo(), nullptr);

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

It would be nice to also test that the alias analysis based transform also works with invoked now.

Comment on lines 494 to 495
assert(isa<CallBase>(U) && "Expected all users of Function to be CallBase");
CallBase *Call = cast<CallBase>(U);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert(isa<CallBase>(U) && "Expected all users of Function to be CallBase");
CallBase *Call = cast<CallBase>(U);
auto *Call = cast<CallBase>(U);

cast performs an implicit isa assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice - updated to remove the superfluous assert(), and added some tests for invoke instructions.

- Remove superfluous assert()
- Add tests for invoke instructions
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@hazzlim hazzlim merged commit 4da4fac into llvm:main Oct 4, 2024
6 of 8 checks passed
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.

3 participants