Skip to content

Conversation

@JamieCunliffe
Copy link
Contributor

@JamieCunliffe JamieCunliffe commented Oct 6, 2025

If the calling function had more target features enabled than the callee than the attribute wasn't being applied as the arguments for the check had been swapped round. Also includes target features that are part of the global set as the warning was checking those but when adding the attribute they were not checked.

Add a codegen-llvm test to check that the attribute is actually applied as previously only the warning was being checked.

Tracking issue: #145574

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 6, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2025

r? @jackh726

rustbot has assigned @jackh726.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

The changes seem okay...but I'm not sure exactly what's changing in the test? I ran this on godbolt with nightly and it compiles? Could be helpful to 1) Add the test in a commit prior to your changes, so can see what changes, and 2) Add some comments to the test on what the expected behavior is an why.

As an aside, trying to think of how to make this not come up again. Maybe worth also adding a doc comment to is_target_feature_call_safe on what the arguments are.

View changes since this review

@JamieCunliffe
Copy link
Contributor Author

I'll make those changes.

But as for what has changed, it was that the #3 attribute wasn't being applied on line 25 and 33. Without that we wasn't requesting the inlining. Depending on what other passes were run though, inlining did happen in some cases which is why I missed it.

@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2025
If the calling function had more target features enabled than the callee than the attribute wasn't being applied as the arguments for the check had been swapped round. Also includes target features that are part of the global set as the warning was checking those but when adding the attribute they were not checked. Add a codegen-llvm test to check that the attribute is actually applied as previously only the warning was being checked.
@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@JamieCunliffe
Copy link
Contributor Author

As an aside, trying to think of how to make this not come up again.

Thinking about that, we emit the warning in a different place to applying the attribute. We had a test that checked the warning but not the codegen, if we emitted the warning here we would have caught it. Although we were still lacking the test that checks the attribute is present in the IR. Something like this could always come up again if one of the 2 checks is changed and the other isn't changed to match.

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

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

3 participants