Skip to content

Re-block SRoA on SIMD types #144665

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 31, 2025
Merged

Re-block SRoA on SIMD types #144665

merged 1 commit into from
Jul 31, 2025
+55 −1

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Jul 30, 2025 •
edited by rustbot
Loading

Fixes #144621

@rustbot
Copy link
Collaborator

rustbot commented Jul 30, 2025

r? @saethlin

rustbot has assigned @saethlin.
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

@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 Jul 30, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 30, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@scottmcm scottmcm mentioned this pull request Jul 30, 2025
Closed
compiler-errors
compiler-errors reviewed Jul 30, 2025
View reviewed changes
compiler/rustc_mir_transform/src/sroa.rs
@@ -72,8 +72,12 @@ fn escaping_locals<'tcx>(
return true;
}
if let ty::Adt(def, _args) = ty.kind()
&& tcx.is_lang_item(def.did(), LangItem::DynMetadata)
&& (def.repr().simd() || tcx.is_lang_item(def.did(), LangItem::DynMetadata))
Copy link
Member

Choose a reason for hiding this comment

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

Please update the doc comment on this function.

Also, could you rename this function? Calling it escaping_locals seems a bit misleading given it has opt-outs for locals that are not necessarily escaping.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@scottmcm scottmcm force-pushed the restore-sroa-simd-check branch 2 times, most recently from d942a9d to ec7e4f9 Compare July 30, 2025 06:53
compiler-errors
compiler-errors reviewed Jul 30, 2025
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I'm pretty confused about how this was refactored. This code distinguishes escaping and excluded, but AFAICT escaping is a superset of the two that shouldn't change. Is there something I'm misunderstanding about the code, or are there further simplifications we can do here?

compiler/rustc_mir_transform/src/sroa.rs Outdated
@@ -31,8 +31,9 @@ impl<'tcx> crate::MirPass<'tcx> for ScalarReplacementOfAggregates {
let mut excluded = excluded_locals(body);
let typing_env = body.typing_env(tcx);
loop {
add_type_based_exclusions(tcx, &mut excluded, body);
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to happen in the loop? The set of excluded locals based off of type exclusions shouldn't change, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we're in a loop I think it can because SRoA might add extra locals in the loop?

We can SRoA a (__m128, __m128), adding new locals which themselves need to not get SRoA'd.

compiler/rustc_mir_transform/src/sroa.rs Outdated
body: &Body<'tcx>,
) -> DenseBitSet<Local> {
) {
let is_excluded_ty = |ty: Ty<'tcx>| {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just inline this closure? It's just a simple match on ADT, even if it's not written like that today.

compiler/rustc_mir_transform/src/sroa.rs Outdated
Comment on lines 100 to 95
let mut set = DenseBitSet::new_empty(body.local_decls.len());
set.insert_range(RETURN_PLACE..=Local::from_usize(body.arg_count));
for (local, decl) in body.local_decls().iter_enumerated() {
if excluded.contains(local) || is_excluded_ty(decl.ty) {
for local in body.local_decls().indices() {
if excluded.contains(local) {
set.insert(local);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is morally just a clone of excluded, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, probably. I'll give it a shot. (I don't know if there's potentially different lengths because of changing numbers of locals.)

@scottmcm
Copy link
Member Author

Actually, @compiler-errors, would you be ok with just doing the first commit here for now -- since beta branches in 2 days and we should have a fix for this -- as a partial revert of #144543, and then doing the refactor in another PR?

@compiler-errors
Copy link
Member

Sure

@compiler-errors
Copy link
Member

r=me on the first commit

@scottmcm scottmcm force-pushed the restore-sroa-simd-check branch from ec7e4f9 to fe08ba0 Compare July 31, 2025 06:32
@scottmcm
Copy link
Member Author

@bors r=compiler-errors

@bors
Copy link
Collaborator

bors commented Jul 31, 2025

📌 Commit fe08ba0 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 Jul 31, 2025
@Zalathar Zalathar mentioned this pull request Jul 31, 2025
Merged
bors added a commit that referenced this pull request Jul 31, 2025
@bors
Auto merge of #144723 - Zalathar:rollup-f9e0rfo, r=Zalathar
64ca23b 
Rollup of 3 pull requests

Successful merges:

 - #144657 (fix: Only "close the window" when its the last annotated file)
 - #144665 (Re-block SRoA on SIMD types)
 - #144713 (`rustc_middle::ty` cleanups)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8cca6bc into rust-lang:master Jul 31, 2025
20 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 31, 2025
rust-timer added a commit that referenced this pull request Jul 31, 2025
@rust-timer
Unrolled build for #144665
eed7cb8 
Rollup merge of #144665 - scottmcm:restore-sroa-simd-check, r=compiler-errors

Re-block SRoA on SIMD types

Fixes #144621
@scottmcm scottmcm deleted the restore-sroa-simd-check branch July 31, 2025 15:38
@scottmcm
Copy link
Member Author

I've been thinking about the structure here and pondering whether there's some useful loop invariants that can simplify the refactor. Notably, since we're already blocking &_3.1 from SRoA'ing _3 at all, I think we don't actually need to check the escaping for the new locals we introduce when expanding a local, since the parts can't be borrowed either.

Still TBD on trying that out and seeing how it feels, though.

@stefanboca stefanboca mentioned this pull request Jul 31, 2025
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

Assignees

@saethlin saethlin

Labels
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
Milestone
1.90.0
Development

Successfully merging this pull request may close these issues.

ICE building glam: Projecting into SIMD type std::arch::aarch64::float32x4_t is banned by MCP#838
6 participants
@scottmcm @rustbot @rust-log-analyzer @compiler-errors @bors @saethlin