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

Fix inlining with -Zalways-encode-mir #115194

Merged
merged 1 commit into from Aug 31, 2023

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Aug 24, 2023

Only inline functions that are considered eligible for inlining
by the reachability pass.

This constraint was previously indirectly enforced by only exporting MIR
of eligible functions, but that approach doesn't work with
-Zalways-encode-mir enabled.

@rustbot rustbot added 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 Aug 24, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 24, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 25, 2023

r? compiler

Comment on lines 391 to 392
// Reachability pass defines which functions are eligible for inlining. Generally inlining
// other functions is incorrect, because they could reference symbols that aren't exported.
Copy link
Member

Choose a reason for hiding this comment

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

This change is coupling the MIR inliner even more to our dubious reachability analysis, and I think that's the wrong way to improve things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does it increase coupling to reachability analysis?

Copy link
Member

Choose a reason for hiding this comment

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

MIR inlining needs to agree with the logic used to generate MIR for items and also with whether statics are considered reachable. If it doesn't, we end up needing MIR for a function that we don't have, or we end up requiring visibility for a static that was previously determined to be internal.

This change as written currently is restricting MIR inlining to match the behavior of this function:

fn item_might_be_inlined(tcx: TyCtxt<'_>, item: &hir::Item<'_>, attrs: &CodegenFnAttrs) -> bool {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course it needs to agree with definition in reachability pass. It was also the case before, so I am quite confused by your comment about increased coupling.

Only inline functions that are considered eligible for inlining
by the reachability pass.

This constraint was previously indirectly enforced by only exporting MIR
of eligible functions, but that approach doesn't work with
-Zalways-encode-mir enabled.
@compiler-errors
Copy link
Member

Yeah, the coupling was always there.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 30, 2023

📌 Commit fe3cd2d has been approved by compiler-errors

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2023
@tmiasko tmiasko force-pushed the inline-always-encode-mir branch 2 times, most recently from 945cde2 to fe3cd2d Compare August 30, 2023 06:30
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 30, 2023

Ups, pushed to a wrong branch.

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Aug 30, 2023

📌 Commit fe3cd2d has been approved by compiler-errors

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 30, 2023

⌛ Testing commit fe3cd2d with merge b1b244d...

@bors
Copy link
Contributor

bors commented Aug 31, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing b1b244d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 31, 2023
@bors bors merged commit b1b244d into rust-lang:master Aug 31, 2023
23 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Aug 31, 2023
@tmiasko tmiasko deleted the inline-always-encode-mir branch August 31, 2023 05:10
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b1b244d): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.2% [0.6%, 1.8%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.4% [-1.4%, -1.4%] 1
Improvements ✅
(secondary)
-3.8% [-3.8%, -3.8%] 1
All ❌✅ (primary) 0.7% [-1.4%, 1.8%] 5

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 633.863s -> 631.655s (-0.35%)
Artifact size: 316.71 MiB -> 316.64 MiB (-0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

None yet

7 participants