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.

[dianqk]

That test documents internally that it is testing for a bug??? Maybe we should delete it in another PR if we can't figure out what it's for.

The test that is from #128627 is correct but fragile.

The test expects the output is:

Breakpoint 1, dummy_span::main () at tests/debuginfo/dummy_span.rs:53
53	    zzz(); // #break
54	    if number % 7 == 0 {
58	        return Ok(()); // #loc1
61	} // #loc2
std::sys::backtrace::__rust_begin_short_backtrace<fn() -> core::result::Result<(), core::num::error::ParseIntError>, core::result::Result<(), core::num::error::ParseIntError>> (f=0x555555555f70 <dummy_span::main>) at /rustc/FAKE_PREFIX/library/std/src/sys/backtrace.rs:172
std::rt::lang_start::{closure#0}<core::result::Result<(), core::num::error::ParseIntError>> () at /rustc/FAKE_PREFIX/library/std/src/rt.rs:206

not:

Breakpoint 1, dummy_span::main () at tests/debuginfo/dummy_span.rs:53
53	    zzz(); // #break
54	    if number % 7 == 0 {
58	        return Ok(()); // #loc1
1	//@ min-lldb-version: 310
61	} // #loc2
std::sys::backtrace::__rust_begin_short_backtrace<fn() -> core::result::Result<(), core::num::error::ParseIntError>, core::result::Result<(), core::num::error::ParseIntError>> (f=0x555555555f70 <dummy_span::main>) at /rustc/FAKE_PREFIX/library/std/src/sys/backtrace.rs:172
std::rt::lang_start::{closure#0}<core::result::Result<(), core::num::error::ParseIntError>> () at /rustc/FAKE_PREFIX/library/std/src/rt.rs:206

I have dug more into why the #loc2 is missing. Taking the example from https://rust.godbolt.org/z/GvToxj6vh.

The assembly code with SimplifyComparisonIntegral is:

.LBB40_21:
        .loc    6 14 16 is_stmt 1
        mov     byte ptr [rsp + 79], 5
        .loc    6 0 0 is_stmt 0
        jmp     .LBB40_23
...
.Ltmp314:
.LBB40_23:
        .loc    6 18 1 is_stmt 1
        lea     rdi, [rsp + 112]
        call    qword ptr [rip + core[270d1373c8f6a07d]::ptr::drop_in_place::<alloc[206d51544a00c3e8]::string::String>@GOTPCREL]
        jmp     .LBB40_27

without SimplifyComparisonIntegral:

.LBB40_22:
        .loc    6 14 16 is_stmt 1
        mov     byte ptr [rsp + 79], 5
        .loc    6 0 0 is_stmt 0
        jmp     .LBB40_27
...
.Ltmp319:
.LBB40_26:
        .loc    6 18 2 is_stmt 0
        mov     al, byte ptr [rsp + 79]
        .loc    6 18 2 epilogue_begin
        add     rsp, 360
        .cfi_def_cfa_offset 8
        ret
.LBB40_27:
        .cfi_def_cfa_offset 368
.Ltmp320:
        .loc    6 18 1 # What about is_stmt here?
        lea     rdi, [rsp + 112]
        call    qword ptr [rip + core[270d1373c8f6a07d]::ptr::drop_in_place::<alloc[206d51544a00c3e8]::string::String>@GOTPCREL]
        jmp     .LBB40_26

IIUC, the .loc of .LBB40_27 is is_stmt 0 because .LBB40_26 is is_stmt 0. I haven't read the DWARF document yet :(.

This can be verified by changing is_stmt 0 to is_stmt 1. Some commands are useful:

llvm-mc -filetype=obj -triple=x86_64 dummy_span.s -o dummy_span.o
llvm-dwarfdump --debug-line dummy_span.o

There is some note from https://llvm.org/docs/KeyInstructionsDebugInfo.html:

DWARF provides a helpful tool the compiler can employ to mitigate this jumpiness, the is_stmt flag, which indicates that an instruction is a recommended breakpoint location. However, LLVM’s current approach to deciding is_stmt placement essentially reduces down to “is the associated line number different to the previous instruction’s?”.

(Note: It’s up to the debugger if it wants to interpret is_stmt or not, and at time of writing LLDB doesn’t;

This tells us:

• is_stmt is not placed if the instruction has the same line number as the previous one.
• LLDB may ignore is_stmt, but GDB may use is_stmt.

I think this has explained why LLDB passes the test. LBB40_26 and LBB40_27 have the same line number, so LBB40_27 doesn't have is_stmt.

I have a more reliable test: https://gist.github.com/dianqk/dd1ce92143bbee077e2af2a3db9c8340, but this can also be fragile when the BBs are reordered due to some changes. I have no objection to removing the test. cc @khuey @nnethercote (#128627)

[khuey]

I'm not sure I follow what the behavior change is ultimately introduced in the test. Are you saying that after this PR stepping through the test in gdb has a jump to line 1?

[dianqk]

I'm not sure I follow what the behavior change is ultimately introduced in the test. Are you saying that after this PR stepping through the test in gdb has a jump to line 1?

No, just gdb skips #loc2:

Breakpoint 1, dummy_span::main () at tests/debuginfo/dummy_span.rs:53
53	    zzz(); // #break
54	    if number % 7 == 0 {
58	        return Ok(()); // #loc1
std::sys::backtrace::__rust_begin_short_backtrace<fn() -> core::result::Result<(), core::num::error::ParseIntError>, core::result::Result<(), core::num::error::ParseIntError>> (f=0x555555555f70 <dummy_span::main>) at /rustc/FAKE_PREFIX/library/std/src/sys/backtrace.rs:172
std::rt::lang_start::{closure#0}<core::result::Result<(), core::num::error::ParseIntError>> () at /rustc/FAKE_PREFIX/library/std/src/rt.rs:206

[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.