Don't optimize across storage markers in SimplifyComparisonIntegral

[qaijuang]

View all comments

This PR rejects SimplifyComparisonIntegral candidates when either StorageLive or StorageDead for the compared local occurs between the comparison and the terminator.

Since those candidates are rejected, the pass no longer relocates StorageDead into switch successors. This conservatively leaves one comparison in issue_59352 unoptimized, so its MIR snapshots are updated.

Resolves #158231.

r? @hanna-kruppe

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[hanna-kruppe]

I don't think scanning only for StorageLive like this is the right fix. As #158231 points out, the StorageDead handling is also incorrect in the presence of loops. In simple cases (like the test case added in this PR), StorageLive and StorageDead both occur in the same block. But there is valid (just not optimal) MIR where this PR doesn't fix the StorageDead problem. For example, consider something like this:

bb1:
    StorageLive(a);
    a = ...;
    goto bb2;
bb2:
    goto bb3;
bb3:
    b = a == 123;
    StorageDead(a);
    switchInt(b) -> [1: bb2, otherwise: bb4]
bb4:
    // ...

This will push the StorageDead(a) into bb2 and bb4, which means a gets uninitialized before the comparison.

I think the simplest and safest bet here is to further limit the optimization so it bails out if there's any StorageDead or StorageLive of the compared local between the comparison and the switchInt.

[hanna-kruppe]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[qaijuang]

I think the simplest and safest bet here is to further limit the optimization so it bails out if there's any StorageDead or StorageLive of the compared local between the comparison and the switchInt.

Done. now rejects both, and removes the old StorageDead successor-relocation machinery entirely

@rustbot ready

[hanna-kruppe]

@rustbot author

[hanna-kruppe]

Thanks, r=me after addressing these nits.

View changes since this review

[hanna-kruppe]

The last commit looks good to me, but (sorry for not realizing this before!) please squash the commits. The simplest thing would be to squash it all into one commit, but if possible it would be great to reorganize this into two commits:

1. First one introduces the new regression tests without fixing the pass (and without CHECK: lines), so the *.SimplifyComparisonIntegral.diff files in this commit show what the pass did wrong before this PR.
2. Second commit fixes the pass logic and adds CHECK: lines to the tests.

Either approach is fine, depends on how confident you are in your git skills :)

[hanna-kruppe]

The first commit is not self contained as requested, the EMIT_MIR output files aren't present and thus tests would fail on that commit. Please update the first commit to fix that.

@rustbot author

[qaijuang]

The first commit is not self contained as requested, the EMIT_MIR output files aren't present and thus tests would fail on that commit. Please update the first commit to fix that.

@rustbot author

ran wit ./x test tests/mir-opt/if_condition_int_storage.rs tests/mir-opt/if_condition_int.rs --bless --force-rerun

got only one output file for tests/mir-opt/if_condition_int_storage.rs, and a panic for tests/mir-opt/if_condition_int.rs:

   --> /rust/tests/mir-opt/if_condition_int.rs:161:13
    |
161 |             c = b == 42;
    |             ^^^^^^^^^^^

note: using internal features is not supported and expected to cause internal compiler errors when used incorrectly

@rustbot ready

[hanna-kruppe]

Oh, the ICE is -Zlint-mir diagnosing the miscompile that the test shows before the fix. Fair enough. Thanks again!

@bors r+ rollup

[rust-bors]

📌 Commit e6b982a has been approved by hanna-kruppe

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

Reason for tree closure: spurious failures

[jhpratt]

@rust-timer build 990bfa8

[rust-timer]

Finished benchmarking commit (990bfa8): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.3%, 0.4%]	2
Regressions ❌ (secondary)	0.3%	[0.1%, 0.6%]	9
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.1%]	2
All ❌✅ (primary)	-0.0%	[-0.8%, 0.4%]	3

Max RSS (memory usage)

Results (primary -7.1%, secondary 1.8%)

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)	2.9%	[2.2%, 3.8%]	3
Improvements ✅ (primary)	-7.1%	[-9.8%, -4.4%]	2
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	-7.1%	[-9.8%, -4.4%]	2

Cycles

Results (primary -3.0%, secondary -1.3%)

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)	3.4%	[2.1%, 4.8%]	4
Improvements ✅ (primary)	-3.0%	[-3.0%, -3.0%]	1
Improvements ✅ (secondary)	-4.3%	[-6.6%, -2.7%]	6
All ❌✅ (primary)	-3.0%	[-3.0%, -3.0%]	1

Binary size

Results (primary 0.2%, secondary 0.4%)

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

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.0%, 0.6%]	15
Regressions ❌ (secondary)	0.4%	[0.0%, 0.7%]	56
Improvements ✅ (primary)	-0.3%	[-0.7%, -0.2%]	7
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	0.2%	[-0.7%, 0.6%]	22

Bootstrap: 487.856s -> 491.177s (0.68%)
Artifact size: 388.05 MiB -> 390.34 MiB (0.59%)