In `simplify_repeated_aggregate`, don't test first element against itself #139411

yotamofek · 2025-04-05T14:02:28Z

r? @saethlin
Noticed that in InstSimplifyContext::simplify_repeated_aggregate, we're accidentally evaluating the first element's value twice, and then comparing it with itself, instead of just checking whether the rest of the elements are equal to the first one.
This will probably save very few cycles, but since InstSimplify is always enabled, this might improve perf by a bit.

…self

rustbot · 2025-04-05T14:02:31Z

Could not assign reviewer from: saethlin.
User(s) saethlin are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

rustbot · 2025-04-05T14:02:33Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
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 · 2025-04-05T14:02:35Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

yotamofek · 2025-04-05T14:02:57Z

compiler/rustc_mir_transform/src/instsimplify.rs

@@ -78,20 +78,20 @@ impl<'tcx> InstSimplifyContext<'_, 'tcx> {
    /// GVN can also do this optimization, but GVN is only run at mir-opt-level 2 so having this in
    /// InstSimplify helps unoptimized builds.
    fn simplify_repeated_aggregate(&self, rvalue: &mut Rvalue<'tcx>) {
-        let Rvalue::Aggregate(box AggregateKind::Array(_), fields) = rvalue else {
+        let Rvalue::Aggregate(box AggregateKind::Array(_), fields) = &*rvalue else {


This is just so fields is immutable, which makes it a bit easier to reason about, IMHO.

Kobzol · 2025-04-05T15:25:41Z

@bors try @rust-timer queue

…ify, r=<try> In `simplify_repeated_aggregate`, don't test first element against itself r? `@saethlin` Noticed that in `InstSimplifyContext::simplify_repeated_aggregate`, we're accidentally evaluating the first element's value twice, and then comparing it with itself, instead of just checking whether the rest of the elements are equal to the first one. This will probably save very few cycles, but since `InstSimplify` is always enabled, this might improve perf by a bit.

bors · 2025-04-05T15:26:53Z

⌛ Trying commit 5b596cd with merge fa2c9b1...

compiler-errors · 2025-04-05T16:11:24Z

r=me (i.e. I will approve) when perf comes back.

bors · 2025-04-05T17:33:23Z

☀️ Try build successful - checks-actions
Build commit: fa2c9b1 (fa2c9b15eafb4a4051228168e5cdfbe2161a2248)

rust-timer · 2025-04-05T18:48:24Z

Finished benchmarking commit (fa2c9b1): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

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

Cycles

Results (secondary 2.3%)

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)	2.3%	[2.3%, 2.3%]	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: 776.909s -> 777.668s (0.10%)
Artifact size: 365.93 MiB -> 365.98 MiB (0.01%)

compiler-errors · 2025-04-05T18:49:20Z

@bors r+ rollup-

bors · 2025-04-05T18:49:23Z

📌 Commit 5b596cd has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2025-04-06T01:45:36Z

⌛ Testing commit 5b596cd with merge 1de9312...

bors · 2025-04-06T04:53:58Z

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

github-actions · 2025-04-06T04:56:04Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing c211076 (parent) -> 1de9312 (this PR)

Test differences

No test diffs found

Job duration changes

x86_64-apple-1: 5974.0s -> 6881.4s (15.2%)
x86_64-msvc-2: 7053.7s -> 7486.8s (6.1%)
dist-aarch64-msvc: 8350.4s -> 8731.8s (4.6%)
x86_64-gnu: 6398.4s -> 6683.8s (4.5%)
dist-x86_64-musl: 7196.6s -> 7431.1s (3.3%)
dist-x86_64-illumos: 5529.0s -> 5702.1s (3.1%)
x86_64-msvc-ext3: 7414.2s -> 7604.9s (2.6%)
x86_64-mingw-2: 7021.1s -> 7194.1s (2.5%)
x86_64-gnu-distcheck: 4698.4s -> 4810.9s (2.4%)
dist-powerpc64-linux: 5293.7s -> 5419.8s (2.4%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-04-06T06:10:20Z

Finished benchmarking commit (1de9312): 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 (secondary -3.3%)

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

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: 775.586s -> 775.764s (0.02%)
Artifact size: 365.94 MiB -> 365.91 MiB (-0.01%)

…ify/simplify_primitive_clone, r=<try> Micro-optimize `InstSimplify`'s `simplify_primitive_clone` r? `@compiler-errors` , since you already did rust-lang#139411 and got randomly selected for rust-lang#139638 (feel free to reassign!) Another one similar in spirit to rust-lang#139411, but this time for `simplify_primitive_clone`, which is doing a bit of redundant work. Might not show up in benches, but probably worth micro-optimizing since the transformation is run even for debug builds. See inline comments for my reasoning for making these changes.

…plify/cleanup, r=oli-obk Cleanup the `InstSimplify` MIR transformation Some minor cleanups and rightward-drift-protection found while working on rust-lang#139411 and a future follow-up

Rollup merge of rust-lang#139638 - yotamofek:pr/mir_transform/instsimplify/cleanup, r=oli-obk Cleanup the `InstSimplify` MIR transformation Some minor cleanups and rightward-drift-protection found while working on rust-lang#139411 and a future follow-up

…plify/simplify_primitive_clone, r=compiler-errors Micro-optimize `InstSimplify`'s `simplify_primitive_clone` r? `@compiler-errors` , since you already did rust-lang#139411 and got randomly selected for rust-lang#139638 (feel free to reassign!) Another one similar in spirit to rust-lang#139411, but this time for `simplify_primitive_clone`, which is doing a bit of redundant work. Might not show up in benches, but probably worth micro-optimizing since the transformation is run even for debug builds. See inline comments for my reasoning for making these changes.

…plify/simplify_primitive_clone, r=compiler-errors Micro-optimize `InstSimplify`'s `simplify_primitive_clone` r? ``@compiler-errors`` , since you already did rust-lang#139411 and got randomly selected for rust-lang#139638 (feel free to reassign!) Another one similar in spirit to rust-lang#139411, but this time for `simplify_primitive_clone`, which is doing a bit of redundant work. Might not show up in benches, but probably worth micro-optimizing since the transformation is run even for debug builds. See inline comments for my reasoning for making these changes.

…plify/simplify_primitive_clone, r=compiler-errors Micro-optimize `InstSimplify`'s `simplify_primitive_clone` r? ```@compiler-errors``` , since you already did rust-lang#139411 and got randomly selected for rust-lang#139638 (feel free to reassign!) Another one similar in spirit to rust-lang#139411, but this time for `simplify_primitive_clone`, which is doing a bit of redundant work. Might not show up in benches, but probably worth micro-optimizing since the transformation is run even for debug builds. See inline comments for my reasoning for making these changes.

…plify/simplify_primitive_clone, r=compiler-errors Micro-optimize `InstSimplify`'s `simplify_primitive_clone` r? ````@compiler-errors```` , since you already did rust-lang#139411 and got randomly selected for rust-lang#139638 (feel free to reassign!) Another one similar in spirit to rust-lang#139411, but this time for `simplify_primitive_clone`, which is doing a bit of redundant work. Might not show up in benches, but probably worth micro-optimizing since the transformation is run even for debug builds. See inline comments for my reasoning for making these changes.

Rollup merge of rust-lang#139644 - yotamofek:pr/mir_transform/instsimplify/simplify_primitive_clone, r=compiler-errors Micro-optimize `InstSimplify`'s `simplify_primitive_clone` r? ````@compiler-errors```` , since you already did rust-lang#139411 and got randomly selected for rust-lang#139638 (feel free to reassign!) Another one similar in spirit to rust-lang#139411, but this time for `simplify_primitive_clone`, which is doing a bit of redundant work. Might not show up in benches, but probably worth micro-optimizing since the transformation is run even for debug builds. See inline comments for my reasoning for making these changes.

In simplify_repeated_aggregate, don't test first element against it…

5b596cd

…self

rustbot assigned petrochenkov Apr 5, 2025

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 Apr 5, 2025

yotamofek commented Apr 5, 2025

View reviewed changes

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 Apr 5, 2025

compiler-errors approved these changes Apr 5, 2025

View reviewed changes

compiler-errors assigned compiler-errors and unassigned petrochenkov Apr 5, 2025

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 Apr 5, 2025

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 Apr 5, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2025

bors merged commit 1de9312 into rust-lang:master Apr 6, 2025
7 checks passed

rustbot added this to the 1.88.0 milestone Apr 6, 2025

yotamofek deleted the pr/mir_transform/instsimplify branch April 6, 2025 08:19

This was referenced Apr 10, 2025

Cleanup the InstSimplify MIR transformation #139638

Merged

Micro-optimize InstSimplify's simplify_primitive_clone #139644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In `simplify_repeated_aggregate`, don't test first element against itself #139411

In `simplify_repeated_aggregate`, don't test first element against itself #139411

yotamofek commented Apr 5, 2025

rustbot commented Apr 5, 2025

rustbot commented Apr 5, 2025

rustbot commented Apr 5, 2025

yotamofek Apr 5, 2025

Kobzol commented Apr 5, 2025

This comment has been minimized.

bors commented Apr 5, 2025

compiler-errors commented Apr 5, 2025

bors commented Apr 5, 2025

This comment has been minimized.

rust-timer commented Apr 5, 2025

compiler-errors commented Apr 5, 2025

bors commented Apr 5, 2025

bors commented Apr 6, 2025

bors commented Apr 6, 2025

github-actions bot commented Apr 6, 2025

rust-timer commented Apr 6, 2025

In simplify_repeated_aggregate, don't test first element against itself #139411

In simplify_repeated_aggregate, don't test first element against itself #139411

Conversation

yotamofek commented Apr 5, 2025

rustbot commented Apr 5, 2025

rustbot commented Apr 5, 2025

rustbot commented Apr 5, 2025

Choose a reason for hiding this comment

Kobzol commented Apr 5, 2025

This comment has been minimized.

bors commented Apr 5, 2025

compiler-errors commented Apr 5, 2025

bors commented Apr 5, 2025

This comment has been minimized.

rust-timer commented Apr 5, 2025

Overall result: no relevant changes - no action needed

compiler-errors commented Apr 5, 2025

bors commented Apr 5, 2025

bors commented Apr 6, 2025

bors commented Apr 6, 2025

github-actions bot commented Apr 6, 2025

Test differences

Job duration changes

rust-timer commented Apr 6, 2025

Overall result: no relevant changes - no action needed

In `simplify_repeated_aggregate`, don't test first element against itself #139411

In `simplify_repeated_aggregate`, don't test first element against itself #139411