Assorted improvements for `rustc_middle::mir::traversal` by WaffleLapkin · Pull Request #116254 · rust-lang/rust

WaffleLapkin · 2023-09-28T23:20:28Z

I'm not entirely sure about all changes, although I do like all of them. If you'd like I can drop some commits. Best reviewed on a commit-by-commit basis, I think, since they are fairly isolated.

There is no `reset` anymore

It was not used anywhere, instead we directly reverse postorder.

The only usage immediately throws out the data, so.

rustbot · 2023-09-28T23:51:25Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

cjgillot · 2023-09-29T06:54:08Z

        // since we've already visited `E`, that child isn't added to the stack. The last
        // two iterations yield `B` and finally `A` for a final traversal of [E, D, C, B, A]
-        while let Some(&mut (_, ref mut iter)) = self.visit_stack.last_mut() && let Some(bb) = iter.next_back() {
+        while let Some(bb) = self.visit_stack.last_mut().and_then(|(_, iter)| iter.next_back()) {


Should this use a let chain ?

I can drop 0e0dc59 if you want :P

My point for using and_then was to show that iter is only used for next_back and is not used in the body of the loop. IMO with let chain it was a bit harder to read

cjgillot · 2023-09-29T06:55:29Z

-/// Construction of a `ReversePostorder` traversal requires doing a full
-/// postorder traversal of the graph, therefore this traversal should be
-/// constructed as few times as possible. Use the `reset` method to be able
-/// to re-use the traversal


This comment is very useful to newcomers that do not know the different types of traversal. Can it be preserved somewhere ?

What do you think about ef3c5bb?

cjgillot · 2023-09-29T06:56:00Z

+    traversal::Postorder::new(&mir.basic_blocks, location.block)
+        .collect::<Vec<_>>()
+        .into_iter()
+        .rev()


Can the cached traversal be used here ?

Not easily I think, this uses a root block setting, but we only cache this for root = START_BLOCK.

cjgillot · 2023-09-30T12:37:04Z

@bors r+

bors · 2023-09-30T12:37:06Z

📌 Commit 814fbd8 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-09-30T12:38:15Z

⌛ Testing commit 814fbd8 with merge 75d731e...

bors · 2023-09-30T14:31:15Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 75d731e to master...

rust-timer · 2023-09-30T15:53:54Z

Finished benchmarking commit (75d731e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	-	-	0
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	2
Improvements ✅ (primary)	-3.6%	[-3.6%, -3.6%]	1
Improvements ✅ (secondary)	-1.3%	[-1.9%, -0.8%]	2
All ❌✅ (primary)	-3.6%	[-3.6%, -3.6%]	1

Cycles

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)	-	-	0
Regressions ❌ (secondary)	4.0%	[4.0%, 4.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 630.884s -> 632.079s (0.19%)
Artifact size: 317.43 MiB -> 317.30 MiB (-0.04%)

…llot Assorted improvements for `rustc_middle::mir::traversal` r? `@cjgillot` I'm not _entirely_ sure about all changes, although I do like all of them. If you'd like I can drop some commits. Best reviewed on a commit-by-commit basis, I think, since they are fairly isolated.

WaffleLapkin added 7 commits September 28, 2023 22:15

Remove outdated comment

f040210

There is no `reset` anymore

Remove ReversePostorder altogether

82e251b

It was not used anywhere, instead we directly reverse postorder.

Simplify Postorder::next

fb0e585

(&mut iter) -> iter.by_ref()

0d8a458

Don't resolve basic block data in Postorder

a7f3c4e

The only usage immediately throws out the data, so.

Remove unnecessary &mut/ref mut pair

e0abb98

Use and_then instead of while let chain to clarify iter scope

0e0dc59

rustbot assigned cjgillot Sep 28, 2023

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

This comment has been minimized.

Sign in to view

Reverse postorder instead of using reversed postorder

2724243

cjgillot reviewed Sep 29, 2023

View reviewed changes

lqd reviewed Sep 29, 2023

View reviewed changes

Comment thread compiler/rustc_middle/src/mir/basic_blocks.rs Outdated

This comment has been minimized.

Sign in to view

rust-cloud-vms Bot force-pushed the nicen-traversal branch from ef3c5bb to 8f7ca0f Compare September 29, 2023 18:42

This comment has been minimized.

Sign in to view

Remove deleted docs + better link together MIR traversing docs

814fbd8

rust-cloud-vms Bot force-pushed the nicen-traversal branch from 8f7ca0f to 814fbd8 Compare September 29, 2023 19:16

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 30, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 30, 2023

bors merged commit 75d731e into rust-lang:master Sep 30, 2023

rustbot added this to the 1.74.0 milestone Sep 30, 2023

pvdrz mentioned this pull request Oct 4, 2023

Fix divergence from upstream master ferrocene/ferrocene#23

Merged

WaffleLapkin deleted the nicen-traversal branch June 1, 2025 13:22

Uh oh!

Conversation

WaffleLapkin commented Sep 28, 2023

Uh oh!

This comment has been minimized.

rustbot commented Sep 28, 2023

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!

Uh oh!

This comment has been minimized.

This comment has been minimized.

cjgillot commented Sep 30, 2023

Uh oh!

bors commented Sep 30, 2023

Uh oh!

bors commented Sep 30, 2023

Uh oh!

bors commented Sep 30, 2023

Uh oh!

rust-timer commented Sep 30, 2023

Overall result: no relevant changes - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants