`rustc_mir_transform` cleanups, round 2 #129929

nnethercote · 2024-09-03T07:48:07Z

More cleanups in the style of #129738.

r? @cjgillot

rustbot · 2024-09-03T07:48:16Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

cjgillot · 2024-09-03T09:57:45Z

compiler/rustc_mir_transform/src/abort_unwinding_calls.rs

-        let mut calls_to_terminate = Vec::new();
-        let mut cleanups_to_remove = Vec::new();
-        for (id, block) in body.basic_blocks.iter_enumerated() {
+        for block in body.basic_blocks.as_mut() {


This changes behaviour: basic_block.as_mut() clears the cfg caches.

Good catch. I will change it to as_mut_preserves_cfg.

I have switched to as_mut_preserves_cfg, but I'm not certain if it's correct.

When you say "changes behaviour", clearing the caches might hurt perf, but won't have any visibile functional effect, right? But switching to as_mut_preserves_cfg could have a visible effect. I can just remove this commit if necessary.

I could just go back to as_mut() and let the cache be cleared, and measure the perf effect. I suspect it will be negligible.

Using this is actually unsound, as you are modifying the CFG.
You need to call body.basic_blocks.invalidate_cfg_cache() explicitly if you use this.
3 options:

keep as-is;

use as_mut() and potentially clear the cache -> probably not an issue, as this pass runs just after ElaborateDrops which changes the CFG a lot, so we may have nothing in cache;

use as_mut_preserves_cfg() and call invalidate_cfg_cache() explicitly -> I'm not super fond, as that's a footgun.

I measured and found that the cache is always totally empty when this code path is hit. So I will convert back to as_mut and I think that should be good enough.

compiler/rustc_mir_transform/src/early_otherwise_branch.rs

cjgillot · 2024-09-03T10:07:27Z

compiler/rustc_mir_transform/src/lower_intrinsics.rs

-                            });
-                            terminator.kind = TerminatorKind::Goto { target };
-                        }
+                        let target = target.unwrap();


I'm a bit split on this commit. This could backfire in case we have a pathological input where MIR builder concluded that we don't need a target block, although we need one.

The existing code uses a mix of fallible and infallible. Can you see a reason for this split? To me it wasn't clear, it looked arbitrary, so I tried making it all infallible and the test suite passed...

The only reason I see is personal taste of the person writing the code.
target can only be None if the returned type is uninhabited. If the type is known, which happens a lot for intrinsics, we can unwrap without any issue. If the type depends on the user code, we can't.

All the intrinsics have a non-! return type: forget, {add,sub,mul}_with_overflow, size_of, min_align_of, and discriminant_value. So I think this change is safe.

nnethercote · 2024-09-03T22:30:38Z

I have updated: I switched to as_mut_preserves_cfg, but I'm not sure about it's correctness. I replaced the len == 0 comparison. I haven't change the target unwrapping. Let me know what you think. Thanks!

bors · 2024-09-05T02:31:21Z

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

bors · 2024-09-06T10:38:19Z

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

Lots of unnecessary `pub`s in this crate. Most are downgraded to `pub(super)`, though some don't need any visibility.

Currently it constructs two vectors `calls_to_terminated` and `cleanups_to_remove` in the main loop, and then processes them after the main loop. But the processing can be done in the main loop, avoiding the need for the vectors.

Let chains are perfect for this kind of function.

Nothing comes after it within the loop.

It doesn't need to be, and it's 72 bytes on 64-bit platforms, which is fairly large.

It's nicer than the `IndexVec` type.

In some cases `target` and `arg` are obtained fallibly, and in some cases they are obtained infallibly. This commit changes them all to infallible.

nnethercote · 2024-09-09T05:33:15Z

I rebased. Now that I've gone back to as_mut, I think this should be ready to merge.

cjgillot · 2024-09-09T13:01:39Z

@bors r+

bors · 2024-09-09T13:01:42Z

📌 Commit 5445953 has been approved by cjgillot

It is now in the queue for this repository.

…eanups-2, r=cjgillot `rustc_mir_transform` cleanups, round 2 More cleanups in the style of rust-lang#129738. r? `@cjgillot`

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#129929 (`rustc_mir_transform` cleanups, round 2) - rust-lang#130022 (Dataflow/borrowck lifetime cleanups) - rust-lang#130064 (fix ICE in CMSE type validation) - rust-lang#130067 (Remove redundant check in `symlink_hard_link` test) - rust-lang#130131 (Print a helpful message if any tests were skipped for being up-to-date) - rust-lang#130137 (Fix ICE caused by missing span in a region error) - rust-lang#130153 (use verbose flag as a default value for `rust.verbose-tests`) - rust-lang#130154 (Stabilize `char::MIN`) - rust-lang#130158 (Update books) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129929 - nnethercote:rustc_mir_transform-cleanups-2, r=cjgillot `rustc_mir_transform` cleanups, round 2 More cleanups in the style of rust-lang#129738. r? ``@cjgillot``

…eanups-3, r=saethlin `rustc_mir_transform` cleanups 3 More cleanups in the style of rust-lang#129929. r? `@saethlin`

Rollup merge of rust-lang#130175 - nnethercote:rustc_mir_transform-cleanups-3, r=saethlin `rustc_mir_transform` cleanups 3 More cleanups in the style of rust-lang#129929. r? `@saethlin`

rustbot assigned cjgillot Sep 3, 2024

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 3, 2024

cjgillot reviewed Sep 3, 2024

View reviewed changes

nnethercote force-pushed the rustc_mir_transform-cleanups-2 branch from 6c39576 to 713a465 Compare September 3, 2024 22:28

nnethercote force-pushed the rustc_mir_transform-cleanups-2 branch from 713a465 to 63376c6 Compare September 5, 2024 00:04

nnethercote force-pushed the rustc_mir_transform-cleanups-2 branch from 63376c6 to a297ee8 Compare September 5, 2024 04:48

nnethercote added 10 commits September 9, 2024 08:48

Reduce visibilities, and add warn(unreachable_pub).

6af470e

Lots of unnecessary `pub`s in this crate. Most are downgraded to `pub(super)`, though some don't need any visibility.

Factor out some more repetitive code.

cd9fd27

Streamline AbortUnwindingCalls.

ec6fe4e

Currently it constructs two vectors `calls_to_terminated` and `cleanups_to_remove` in the main loop, and then processes them after the main loop. But the processing can be done in the main loop, avoiding the need for the vectors.

Simplify verify_candidate_branch.

cc09ab3

Let chains are perfect for this kind of function.

Use let/else to de-indent ElaborateBoxDerefs::run_pass.

181fbd5

Remove an unnecessary continue.

4f2588f

Nothing comes after it within the loop.

Remove some unnecessary dereferences.

9cf90b9

Make CallSite non-Copy.

7adde3f

It doesn't need to be, and it's 72 bytes on 64-bit platforms, which is fairly large.

Use LocalDecls in a couple of places.

751c8b4

It's nicer than the `IndexVec` type.

Improve consistency in LowerIntrinsics.

5445953

In some cases `target` and `arg` are obtained fallibly, and in some cases they are obtained infallibly. This commit changes them all to infallible.

nnethercote force-pushed the rustc_mir_transform-cleanups-2 branch from a297ee8 to 5445953 Compare September 9, 2024 05:32

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 Sep 9, 2024

This was referenced Sep 9, 2024

Rollup of 9 pull requests #130163

Closed

Rollup of 9 pull requests #130165

Merged

bors merged commit 3e3b148 into rust-lang:master Sep 9, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Sep 9, 2024

nnethercote deleted the rustc_mir_transform-cleanups-2 branch September 9, 2024 22:37

nnethercote mentioned this pull request Sep 9, 2024

rustc_mir_transform cleanups 3 #130175

Merged

DianQK mentioned this pull request Sep 18, 2024

Apply EarlyOtherwiseBranch to scalar value #129047

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rustc_mir_transform` cleanups, round 2 #129929

`rustc_mir_transform` cleanups, round 2 #129929

nnethercote commented Sep 3, 2024

rustbot commented Sep 3, 2024

cjgillot Sep 3, 2024

nnethercote Sep 3, 2024

nnethercote Sep 3, 2024

nnethercote Sep 4, 2024

cjgillot Sep 6, 2024 •

edited

Loading

nnethercote Sep 9, 2024

cjgillot Sep 3, 2024

nnethercote Sep 3, 2024

cjgillot Sep 6, 2024

nnethercote Sep 9, 2024

nnethercote commented Sep 3, 2024

bors commented Sep 5, 2024

bors commented Sep 6, 2024

nnethercote commented Sep 9, 2024

cjgillot commented Sep 9, 2024

bors commented Sep 9, 2024

rustc_mir_transform cleanups, round 2 #129929

rustc_mir_transform cleanups, round 2 #129929

Conversation

nnethercote commented Sep 3, 2024

rustbot commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnethercote commented Sep 3, 2024

bors commented Sep 5, 2024

bors commented Sep 6, 2024

nnethercote commented Sep 9, 2024

cjgillot commented Sep 9, 2024

bors commented Sep 9, 2024

`rustc_mir_transform` cleanups, round 2 #129929

`rustc_mir_transform` cleanups, round 2 #129929

cjgillot Sep 6, 2024 •

edited

Loading