Fix issue with callsite inline attribute not being applied sometimes. #147404

JamieCunliffe · 2025-10-06T15:21:08Z

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 · 2025-10-06T15:21:13Z

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

jackh726

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 · 2025-10-06T16:09:55Z

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.

JamieCunliffe · 2025-10-27T16:13:36Z

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.

jackh726 · 2025-11-03T15:27:29Z

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 · 2025-11-03T15:27:32Z

✌️ @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

JamieCunliffe · 2025-11-07T17:16:14Z

@bors r=@jackh726

bors · 2025-11-07T17:16:17Z

📌 Commit e2c5c18 has been approved by jackh726

It is now in the queue for this repository.

…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

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 · 2025-11-08T08:03:55Z

@bors r- #148681 (comment)

rustbot · 2025-11-24T18:22:40Z

This PR was rebased onto a different main 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.

jackh726 · 2025-11-24T18:55:49Z

@JamieCunliffe can you squash the last two commits into the other two (in whatever way is appropriate)

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.

davidtwco · 2025-11-28T10:43:41Z

@bors r=jackh726

bors · 2025-11-28T10:43:43Z

📌 Commit d9ed836 has been approved by jackh726

It is now in the queue for this repository.

matthiaskrgr · 2025-11-28T14:18:55Z

@bors rollup=iffy

bors · 2025-11-28T22:58:25Z

⌛ Testing commit d9ed836 with merge 1eb0657...

bors · 2025-11-29T02:07:44Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 1eb0657 to main...

github-actions · 2025-11-29T02:10:47Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing cc3eee7 (parent) -> 1eb0657 (this PR)

Test differences

Show 4 test diffs

Stage 1

[codegen] tests/codegen-llvm/inline-always-callsite.rs: [missing] -> pass (J0)

Stage 2

[codegen] tests/codegen-llvm/inline-always-callsite.rs: [missing] -> pass (J1)

Additionally, 2 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 1eb0657f78777f0b4d6bcc49c126d5d35212cae5 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

aarch64-apple: 7442.7s -> 9870.3s (+32.6%)
dist-aarch64-apple: 6610.9s -> 8173.3s (+23.6%)
x86_64-gnu-llvm-20: 2361.8s -> 2752.0s (+16.5%)
pr-check-1: 1673.6s -> 1943.2s (+16.1%)
x86_64-rust-for-linux: 2570.9s -> 2972.7s (+15.6%)
x86_64-gnu-gcc: 2999.2s -> 3392.3s (+13.1%)
dist-aarch64-msvc: 6591.6s -> 5760.4s (-12.6%)
aarch64-gnu-llvm-20-1: 3377.3s -> 3787.1s (+12.1%)
i686-gnu-1: 7500.5s -> 8370.2s (+11.6%)
i686-gnu-nopt-1: 7303.0s -> 8130.3s (+11.3%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-11-29T03:33:09Z

Finished benchmarking commit (1eb0657): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.3%, 0.4%]	2
Regressions ❌ (secondary)	0.2%	[0.1%, 0.3%]	5
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	2
Improvements ✅ (secondary)	-0.2%	[-0.4%, -0.1%]	22
All ❌✅ (primary)	0.1%	[-0.1%, 0.4%]	4

Max RSS (memory usage)

Results (secondary -4.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.6%	[-4.6%, -4.6%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.4%	[4.4%, 4.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.6%, -2.2%]	2
All ❌✅ (primary)	-	-	0

Binary size

Results (secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	-	-	0

Bootstrap: 472.717s -> 472.886s (0.04%)
Artifact size: 386.88 MiB -> 386.93 MiB (0.01%)

rustbot assigned jackh726 Oct 6, 2025

jackh726 reviewed Oct 6, 2025

View reviewed changes

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

JamieCunliffe force-pushed the inline-always branch from 0381f59 to 621604d Compare October 27, 2025 16:10

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

JamieCunliffe force-pushed the inline-always branch from 621604d to e2c5c18 Compare November 7, 2025 17:13

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

jhpratt mentioned this pull request Nov 8, 2025

Rollup of 15 pull requests #148681

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 8, 2025

Broken test to show alwaysinline attribute not being applied

95b6747

JamieCunliffe force-pushed the inline-always branch from e2c5c18 to ae078da Compare November 24, 2025 18:22

JamieCunliffe force-pushed the inline-always branch from ae078da to d9ed836 Compare November 26, 2025 13:38

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 28, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 29, 2025

bors merged commit 1eb0657 into rust-lang:main Nov 29, 2025
12 checks passed

rustbot added this to the 1.93.0 milestone Nov 29, 2025

rustbot added the perf-regression Performance regression. label Nov 29, 2025

Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Dec 3, 2025

Fix issue with callsite inline attribute not being applied sometimes. #147404

Fix issue with callsite inline attribute not being applied sometimes. #147404

Conversation

JamieCunliffe commented Oct 6, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 6, 2025

Uh oh!

jackh726 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamieCunliffe commented Oct 6, 2025

Uh oh!

This comment has been minimized.

JamieCunliffe commented Oct 27, 2025

Uh oh!

This comment was marked as resolved.

jackh726 commented Nov 3, 2025

Uh oh!

bors commented Nov 3, 2025

Uh oh!

This comment has been minimized.

JamieCunliffe commented Nov 7, 2025

Uh oh!

bors commented Nov 7, 2025

Uh oh!

This comment has been minimized.

jhpratt commented Nov 8, 2025

Uh oh!

rustbot commented Nov 24, 2025

Uh oh!

jackh726 commented Nov 24, 2025

Uh oh!

davidtwco commented Nov 28, 2025

Uh oh!

bors commented Nov 28, 2025

Uh oh!

matthiaskrgr commented Nov 28, 2025

Uh oh!

bors commented Nov 28, 2025

Uh oh!

bors commented Nov 29, 2025

Uh oh!

Uh oh!

github-actions bot commented Nov 29, 2025

Test differences

Stage 1

Stage 2

Job duration changes

Uh oh!

rust-timer commented Nov 29, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

JamieCunliffe commented Oct 6, 2025 •

edited by rustbot

Loading

jackh726 left a comment •

edited by rustbot

Loading