Use GrowableBitSet in the main loop of `rustc_mir_transform::sroa` by Zalathar · Pull Request #162623 · rust-lang/rust

Zalathar · 2026-09-11T05:38:40Z

Follow-up to Use Box<[Word]> for word storage in DenseBitSet #161957

The first commit is a cleanup based on #161957 (comment) by @panstromek. After having disconnected GrowableBitSet from DenseBitSet and removed unused methods, there is no longer any reason to keep track of a separate domain_size in GrowableBitSet.

After that, I wanted to get rid of DenseBitSet::enlarge, since having it around is not a good fit for the intended purpose of DenseBitSet as a fixed-domain set. After looking more closely at the code in rustc_mir_transform::sroa that uses it, I concluded that that code would be better off using GrowableBitSet instead. GrowableBitSet is just as dense, but avoids the need to set a fixed domain size in advance.

The changes to SROA require adding GrowableBitSet::union, which is easier after having removed domain_size.

(I am not deeply familiar with the SROA pass, but its usage of bitsets seems fairly straightforward in this case.)

There should be no change to compiler output.

rustbot · 2026-09-11T05:38:44Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustbot · 2026-09-11T05:38:47Z

r? @hanna-kruppe

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

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 76 candidates
Random selection from 22 candidates

Zalathar · 2026-09-11T05:39:21Z

This is not perf-motivated, but let's check perf.

@bors try @rust-timer queue

Use GrowableBitSet in the main loop of `rustc_mir_transform::sroa`

rust-bors · 2026-09-11T06:39:57Z

☀️ Try build successful (CI)
Build commit: f34c9f4 (f34c9f4c0d3e95b495f2275c4facc80a42a56f80)
Base parent: 9e9544d (9e9544da4cd757c74428bad35059cc8122fdcd7d)

rust-timer · 2026-09-11T07:24:28Z

Finished benchmarking commit (f34c9f4): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.4%, secondary -3.0%)

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

	mean	range	count
Regressions ❌ (primary)	1.8%	[0.5%, 7.2%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-7.1%	[-7.1%, -7.1%]	1
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	0.4%	[-7.1%, 7.2%]	6

Cycles

Results (primary -0.0%)

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

	mean	range	count
Regressions ❌ (primary)	1.2%	[0.4%, 2.8%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.2%, -0.5%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-1.2%, 2.8%]	14

Binary size

Results (primary -0.0%, 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)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.1%, -0.0%]	45
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	8
All ❌✅ (primary)	-0.0%	[-0.1%, -0.0%]	45

Bootstrap: 478.978s -> 477.993s (-0.21%)
Artifact size: 403.72 MiB -> 403.78 MiB (0.02%)

Zalathar · 2026-09-11T09:51:37Z

Perf came back neutral, so removing the rollup=never.

@bors rollup=maybe

hanna-kruppe

Some of the things SROA does with these bit sets seem a bit silly to me, but that's pre-existing, and I agree that a growable bit set is more natural for what it is doing. The GrowableBitSet cleanup and size_of reduction are also nice.

r=me with small suggestions

View changes since this review

hanna-kruppe · 2026-09-12T10:20:25Z

-    for (local, replacements) in replacements.fragments.iter_enumerated() {
+) -> GrowableBitSet<Local> {
+    let mut all_dead_locals = GrowableBitSet::new_empty();
+    // Fill the GrowableBitSet in reverse so that it only allocates once.


The reason for doing this instead of just starting with GrowableBitSet::with_capacity(num_locals) is to make the early-out (no replacements) case cheaper, right?

(And I guess slightly less work if the largest-numbered local with replacement is much less than num_locals, but if that mattered a lot then ReplacementMap should probably track the maximum.)

Mostly asking to confirm my understanding, but if this is right it might be useful to extend the comment. (And if I got it wrong, the comment should very likely be clarified!)

To be honest I was mostly thinking of the general case, where the highest-numbered replacement is much less than the number of locals, and felt very clever when I noticed that reverse iteration would always allocate the perfect number of words. 😅

But the no-allocation case is also worth noting, so I've updated the comment.

(All of this is not super important to the intent of the PR; it's just some drive-by mini-optimizations that possibly don't even matter in practice.)

I wonder if we need to construct the bit set here at all. It seems like we could avoid iterating over the whole map by already building the bit set in compute_flattening as part of ReplacementMap . It even seems possible to carry over the "only allocate enough words for the dead locals" trick over to that function.

But more generally, the pass seems fairly wasteful in its use of bit sets and IndexVecs, creating everything from scratch in every iteration and from the second iteration only focusing on the newly added locals that have the highest indices. Maybe it doesn't matter in practice, but I wonder if there's a more elegant formulation that maintains state across iterations and puts new locals and changed statements into a worklist.

hanna-kruppe · 2026-09-12T10:28:11Z


+#[test]
+fn growable_union() {
+    // Create two input sets with partly-overlapping values, and different sizes.


Optional nit: maybe add an assert that words.len() is indeed different? It's fairly obvious that they'll be different looking at the current implementation, but if that changes (e.g. eagerly initialize more words than strictly needed to reduce frequent tiny memsets) then the test may become less useful than intended.

hanna-kruppe · 2026-09-12T10:36:02Z

-            self.domain_size = min_domain_size;
-        }
-
+    fn ensure(&mut self, min_domain_size: usize) {


Since "domain size" isn't a thing any more, I'd suggest:

Rename the parameter from min_domain_size to something like min_len or min_bits

Change the doc comment to talk about "allocated and reserved" as in ensure_words, rather than "set can hold n elements"

rustbot · 2026-09-12T10:59:54Z

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.

After the separation from `DenseBitSet` and the removal of unused methods, growable bitsets have no need to track an explicit domain size separate from the length of their storage vec.

The bitset entries tracked by this code don't have a naturally fixed domain, so using GrowableBitSet makes more sense than repeatedly resizing a DenseBitSet.

hanna-kruppe · 2026-09-12T11:30:47Z

r=me once CI is green

Zalathar · 2026-09-12T13:18:55Z

PR CI is green.

@bors r=hanna-kruppe

rust-bors · 2026-09-12T13:18:57Z

📌 Commit 16cd825 has been approved by hanna-kruppe

It is now in the queue for this repository.

…uwer Rollup of 6 pull requests Successful merges: - #162623 (Use GrowableBitSet in the main loop of `rustc_mir_transform::sroa`) - #162678 (Disconnect `rustc_codegen_ssa` from `rustc_mir_transform`) - #162641 (Remove unused arguments of functions) - #162663 (Rustdoc issue template: fix typo in comment ("thorugh")) - #162684 (Add `E0747` explanation for `type/const` mismatch case) - #162685 (Stabilize `Vec::from_fn`)

@panstromek

Rollup merge of #162623 - Zalathar:growable, r=hanna-kruppe Use GrowableBitSet in the main loop of `rustc_mir_transform::sroa` - Follow-up to #161957 --- The first commit is a cleanup based on #161957 (comment) by @panstromek. After having disconnected GrowableBitSet from DenseBitSet and removed unused methods, there is no longer any reason to keep track of a separate `domain_size` in GrowableBitSet. After that, I wanted to get rid of `DenseBitSet::enlarge`, since having it around is not a good fit for the intended purpose of DenseBitSet as a fixed-domain set. After looking more closely at the code in `rustc_mir_transform::sroa` that uses it, I concluded that that code would be better off using GrowableBitSet instead. GrowableBitSet is just as dense, but avoids the need to set a fixed domain size in advance. The changes to SROA require adding `GrowableBitSet::union`, which is easier after having removed `domain_size`. (I am not deeply familiar with the SROA pass, but its usage of bitsets seems fairly straightforward in this case.) There should be no change to compiler output.

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 Sep 11, 2026

rustbot assigned hanna-kruppe Sep 11, 2026

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 11, 2026

This comment has been minimized.

Sign in to view

rust-bors Bot pushed a commit that referenced this pull request Sep 11, 2026

Auto merge of #162623 - Zalathar:growable, r=<try>

f34c9f4

Use GrowableBitSet in the main loop of `rustc_mir_transform::sroa`

This comment has been minimized.

Sign in to view

Zalathar force-pushed the growable branch from f72bd0a to 42fc7cf Compare September 11, 2026 06:15

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 11, 2026

hanna-kruppe approved these changes Sep 12, 2026

View reviewed changes

Zalathar force-pushed the growable branch from 42fc7cf to 7b75dda Compare September 12, 2026 10:59

Zalathar added 2 commits September 12, 2026 21:04

Remove domain_size from GrowableBitSet

0a612a2

After the separation from `DenseBitSet` and the removal of unused methods, growable bitsets have no need to track an explicit domain size separate from the length of their storage vec.

Use GrowableBitSet in the main loop of rustc_mir_transform::sroa

16cd825

The bitset entries tracked by this code don't have a naturally fixed domain, so using GrowableBitSet makes more sense than repeatedly resizing a DenseBitSet.

Zalathar force-pushed the growable branch from 7b75dda to 16cd825 Compare September 12, 2026 11:26

rust-bors Bot 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 Sep 12, 2026

JonathanBrouwer mentioned this pull request Sep 12, 2026

Rollup of 6 pull requests #162687

Merged

rust-bors Bot merged commit 83025dc into rust-lang:main Sep 12, 2026
13 checks passed

rustbot added this to the 1.100.0 milestone Sep 12, 2026

Uh oh!

Uh oh!

Conversation

Zalathar commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 11, 2026

Uh oh!

rustbot commented Sep 11, 2026

Uh oh!

Zalathar commented Sep 11, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rust-bors Bot commented Sep 11, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Sep 11, 2026

Overall result: no relevant changes - no action needed

Uh oh!

Zalathar commented Sep 11, 2026

Uh oh!

hanna-kruppe 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 12, 2026

Uh oh!

hanna-kruppe commented Sep 12, 2026

Uh oh!

Zalathar commented Sep 12, 2026

Uh oh!

rust-bors Bot commented Sep 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Zalathar commented Sep 11, 2026 •

edited

Loading

hanna-kruppe left a comment •

edited by rustbot

Loading