Only use SSA locals in SimplifyComparisonIntegral

[dianqk]

Fixes #150904.

The place may be modified from the comparison statement to the switchInt terminator.

Best reviewed commit by commit.

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

r? @SparrowLii

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

[dianqk]

r? @saethlin (I think you may be interested.)

[Noratrieb]

#75370 (comment) is this still true? (edit: yes)

[Noratrieb]

Looking at #75144 which suggested the pass it does look like it's beneficial for Cranelift but entirely useless for LLVM (also confirmed by https://godbolt.org/z/7jMsGjYvP).

I suggest we delete this pass and let Cranelift implement this optimization on their side, their ISLE framework should be much better suited for these transforms than our MIR opts.

[dianqk]

I have no objection to removing this pass since it also isn't beneficial for other passes, but perhaps the pass can be removed after Cranelift implements the optimization. cc @bjorn3

[saethlin]

SsaLocals is not a trivial amount of analysis, so using it here will need a perf run.

But I agree that SsaLocals and removing the pass are the only real options here.

[Noratrieb]

I have no objection to removing this pass since it also isn't beneficial for other passes, but perhaps the pass can be removed after Cranelift implements the optimization. cc @bjorn3

Given the current status of the Cranelift backend (not production ready) I see no reason to block removal on Cranelift implementing anything. That can happen at a later point just fine.

[bjorn3]

If this pass is giving trouble, it is fine to remove it by me. I can implement a replacement in cg_clif if necessary, but no need to block on that.

[saethlin]

@bors try @rust-timer queue

[rust-bors]

🔒 Merge conflict

This pull request and the base branch diverged in a way that cannot
be automatically merged. Please rebase on top of the latest base
branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository,
you can resolve the conflict following these steps:

1. git checkout if-cmp (switch to your branch)
2. git fetch upstream HEAD (retrieve the latest base branch)
3. git rebase upstream/HEAD -p (rebase on top of it)
4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
5. git push self if-cmp --force-with-lease (update this PR)

You may also read
Git Rebasing to Resolve Conflicts by Drew Blessing
for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub.
It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is
handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

[rustbot]

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.

[khuey]

Ah that's interesting. Based on your other example it seems like your impl Drop for Foo is still attributed to #loc2. Do we understand why std::env::Args's Drop impl isn't?

I'm fine with changing the test to add our own struct with a custom Drop impl if we need it to trigger something on #loc2

[rust-bors]

☀️ Try build successful (CI)
Build commit: eb21be0 (eb21be0dc17f82de5068730d2a1f0f279b8a8209, parent: a6acf0f07f0ed1c12e26dc0db3b9bf1d0504a0bb)

[dianqk]

Ah that's interesting. Based on your other example it seems like your impl Drop for Foo is still attributed to #loc2. Do we understand why std::env::Args's Drop impl isn't?

I'm fine with changing the test to add our own struct with a custom Drop impl if we need it to trigger something on #loc2

std::env::Args also works. I guess it's just a reorder BBs problem that causes the is_stmt flag to be missing. This is a minor bug to LLVM.

[dianqk]

@bors retry (potential flaky test)

[dianqk]

The job i686-msvc-1 failed! Check out the build log: (web) (plain enhanced) (plain)
Click to see the possible cause of the failure (guessed by this bot)

@jieyouxu The failure is similar to #142563.

[jieyouxu]

The job i686-msvc-1 failed! Check out the build log: (web) (plain enhanced) (plain)
Click to see the possible cause of the failure (guessed by this bot)

@jieyouxu The failure is similar to #142563.

Interesting, the middle portion of the baseline ICE message is just

4:  0x8eafad8 - <unknown>

I'll disable it on i686 msvc again

EDIT: #151185

[rust-bors]

☀️ Test successful - CI
Approved by: saethlin
Pushing 18ae990 to main...

[github-actions]

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 22c74ba (parent) -> 18ae990 (this PR)

Test differences

Show 11 test diffs

11 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 18ae99075575810a158cc670dcc7579f1c2ca012 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. aarch64-apple: 12791.3s -> 10606.8s (-17.1%)
2. dist-aarch64-apple: 8888.2s -> 7446.6s (-16.2%)
3. x86_64-gnu-llvm-21-2: 5220.8s -> 6033.9s (+15.6%)
4. dist-x86_64-apple: 8720.6s -> 7598.9s (-12.9%)
5. pr-check-1: 2053.6s -> 1799.6s (-12.4%)
6. x86_64-msvc-ext2: 6212.2s -> 5549.9s (-10.7%)
7. i686-msvc-1: 10822.9s -> 9798.1s (-9.5%)
8. aarch64-gnu: 7266.6s -> 7954.4s (+9.5%)
9. dist-aarch64-msvc: 5836.7s -> 6386.7s (+9.4%)
10. aarch64-gnu-debug: 4081.4s -> 4431.9s (+8.6%)

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]

Finished benchmarking commit (18ae990): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2%, 0.4%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.4%]	3

Max RSS (memory usage)

Results (primary -0.9%, secondary -2.4%)

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

	mean	range	count
Regressions ❌ (primary)	2.9%	[1.7%, 4.5%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.8%	[-5.4%, -1.0%]	6
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.3%]	2
All ❌✅ (primary)	-0.9%	[-5.4%, 4.5%]	9

Cycles

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

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.1%	[0.0%, 0.3%]	22
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	6
Improvements ✅ (primary)	-0.2%	[-0.9%, -0.1%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.9%, 0.3%]	31

Bootstrap: 471.202s -> 471.758s (0.12%)
Artifact size: 383.57 MiB -> 383.58 MiB (0.00%)

[dianqk]

#151244 refines the test.