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
@rustbot

This comment has been minimized.

@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.

@bors
Copy link
Collaborator

bors commented Nov 3, 2025

☔ The latest upstream changes (presumably #148420) made this pull request unmergeable. Please resolve the merge conflicts.

@jackh726
Copy link
Member

jackh726 commented Nov 3, 2025

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.

To be clear, I was specifically referring to is_target_feature_call_safe, and the fact that the two arguments are identical types and they were switched prior to this PR. The hammer approach to this is to put both args in newtype wrappers, but I don't like that. I guess, a comment should be okay.

I'm happy to land this once rebased. So, r=me

@bors delegate+

@bors
Copy link
Collaborator

bors commented Nov 3, 2025

✌️ @JamieCunliffe, you can now approve this pull request!

If @jackh726 told you to "r=me" after making some further change, please make that change, then do @bors r=@jackh726

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 Nov 7, 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

@bors r=@jackh726

@bors
Copy link
Collaborator

bors commented Nov 7, 2025

📌 Commit e2c5c18 has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2025
@rust-log-analyzer
Copy link
Collaborator

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
spellchecking files
building external tool typos from package [email protected]
finished building tool typos
npm WARN deprecated [email protected]: < 24.10.2 is no longer supported
npm ERR! code 127
npm ERR! git dep preparation failed
npm ERR! command /node/bin/node /node/lib/node_modules/npm/bin/npm-cli.js install --force --cache=/home/user/.npm --prefer-offline=false --prefer-online=false --offline=false --no-progress --no-save --no-audit --include=dev --include=peer --include=optional --no-package-lock-only --no-dry-run
npm ERR! npm WARN using --force Recommended protections disabled.
npm ERR! npm WARN deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm ERR! npm WARN deprecated [email protected]: Glob versions prior to v9 are no longer supported
npm ERR! npm WARN deprecated [email protected]: Glob versions prior to v9 are no longer supported
npm ERR! npm WARN deprecated [email protected]: This functionality has been moved to @npmcli/fs
npm ERR! npm WARN deprecated [email protected]: Package no longer supported. Contact Support at https://www.npmjs.com/support for more info.
npm ERR! npm WARN deprecated [email protected]: This package is no longer supported. Please use @npmcli/package-json instead.
npm ERR! npm WARN deprecated [email protected]: This package is no longer supported.
npm ERR! npm ERR! code 127
npm ERR! npm ERR! path /home/user/.npm/_cacache/tmp/git-cloneXXXXXXUiRe35/node_modules/rollup
npm ERR! npm ERR! command failed
npm ERR! npm ERR! command sh -c patch-package
npm ERR! npm ERR! sh: 1: patch-package: not found
npm ERR! 
npm ERR! npm ERR! A complete log of this run can be found in: /home/user/.npm/_logs/2025-11-07T17_20_20_475Z-debug-0.log

npm ERR! A complete log of this run can be found in: /home/user/.npm/_logs/2025-11-07T17_20_13_011Z-debug-0.log
npm install did not exit successfully
tidy [extra_checks]: IO error: npm install returned exit code exit status: 127
tidy [extra_checks]: FAIL
tidy: The following check failed: extra_checks
Bootstrap failed while executing `test src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`
Command `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools-bin/rust-tidy /checkout /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo /checkout/obj/build 4 /node/bin/npm --extra-checks=py,cpp,js,spellcheck` failed with exit code 1
Created at: src/bootstrap/src/core/build_steps/tool.rs:1549:23
Executed at: src/bootstrap/src/core/build_steps/test.rs:1279:29

Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:02:59
  local time: Fri Nov  7 17:20:40 UTC 2025
  network time: Fri, 07 Nov 2025 17:20:40 GMT
##[error]Process completed with exit code 1.
##[group]Run echo "disk usage:"

jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 8, 2025
…kh726

Fix issue with callsite inline attribute not being applied sometimes.

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: rust-lang#145574
bors added a commit that referenced this pull request Nov 8, 2025
Rollup of 15 pull requests

Successful merges:

 - #147404 (Fix issue with callsite inline attribute not being applied sometimes.)
 - #147534 (Implement SIMD funnel shifts in const-eval/Miri)
 - #147686 (update isolate_highest_one for NonZero<T>)
 - #148020 (Show backtrace on allocation failures when possible)
 - #148204 (Modify contributor email entries in .mailmap)
 - #148230 (rustdoc: Properly highlight shebang, frontmatter & weak keywords in source code pages and code blocks)
 - #148555 (Fix rust-by-example spanish translation)
 - #148556 (Fix suggestion for returning async closures)
 - #148585 ([rustdoc] Replace `print` methods with functions to improve code readability)
 - #148600 (re-use `self.get_all_attrs` result for pass indirectly attribute)
 - #148612 (Add note for identifier with attempted hygiene violation)
 - #148613 (Switch hexagon targets to rust-lld)
 - #148644 ([bootstrap] Make `--open` option work with `doc src/tools/error_index_generator`)
 - #148649 (don't completely reset `HeadUsages`)
 - #148675 (Remove eslint-js from npm dependencies)

r? `@ghost`
`@rustbot` modify labels: rollup
@jhpratt
Copy link
Member

jhpratt commented Nov 8, 2025

@bors r- #148681 (comment)

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 8, 2025
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 8, 2025
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants