Only use SSA locals in SimplifyComparisonIntegral #150925
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Collaborator
|
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
Member
Author
|
r? @saethlin (I think you may be interested.) |
Member
|
#75370 (comment) is this still true? (edit: yes) |
Member
|
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. |
Member
Author
|
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 |
This comment has been minimized.
This comment has been minimized.
Member
|
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. |
Member
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. |
Member
|
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. |
Member
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Contributor
|
🔒 Merge conflict This pull request and the base branch diverged in a way that cannot How do I rebase?Assuming
You may also read Please avoid the "Resolve conflicts" button on GitHub. Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how |
Collaborator
|
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. |
This comment has been minimized.
This comment has been minimized.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Contributor
|
💔 Test for 07d45e4 failed: CI. Failed jobs:
|
Member
|
@bors retry (known flaky test that is being reverted) |
rust-bors bot
pushed a commit
that referenced
this pull request
Jan 15, 2026
Revert "avoid phi node for pointers flowing into Vec appends #130998" This reverts PR #130998 because the added test seems to be flaky / non-deterministic, and has been failing in unrelated PRs during merge CI: - #151129 (comment) - #150772 (comment) - #150925 (comment) See also [#t-infra > Tree ops](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Tree.20ops/with/568111767). > [!NOTE] > > This is a "fallback" PR in case the FileCheck failure isn't obvious (i.e. fix-forward). This PR reverts #130998 wholesale in case the failure is genuine and indicative of a bug in the actual implementation change.
Zalathar
added a commit
to Zalathar/rust
that referenced
this pull request
Jan 15, 2026
Revert "avoid phi node for pointers flowing into Vec appends rust-lang#130998" This reverts PR rust-lang#130998 because the added test seems to be flaky / non-deterministic, and has been failing in unrelated PRs during merge CI: - rust-lang#151129 (comment) - rust-lang#150772 (comment) - rust-lang#150925 (comment) - rust-lang#151145 (comment) See also [#t-infra > Tree ops](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Tree.20ops/with/568111767). > [!NOTE] > > This is a "fallback" PR in case the FileCheck failure isn't obvious (i.e. fix-forward). This PR reverts rust-lang#130998 wholesale in case the failure is genuine and indicative of a bug in the actual implementation change.
rust-timer
added a commit
that referenced
this pull request
Jan 15, 2026
Rollup merge of #151150 - revert-vec-append, r=Zalathar Revert "avoid phi node for pointers flowing into Vec appends #130998" This reverts PR #130998 because the added test seems to be flaky / non-deterministic, and has been failing in unrelated PRs during merge CI: - #151129 (comment) - #150772 (comment) - #150925 (comment) - #151145 (comment) See also [#t-infra > Tree ops](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Tree.20ops/with/568111767). > [!NOTE] > > This is a "fallback" PR in case the FileCheck failure isn't obvious (i.e. fix-forward). This PR reverts #130998 wholesale in case the failure is genuine and indicative of a bug in the actual implementation change.
This comment has been minimized.
This comment has been minimized.
rust-bors bot
pushed a commit
that referenced
this pull request
Jan 15, 2026
Only use SSA locals in SimplifyComparisonIntegral Fixes #150904. The place may be modified from the comparison statement to the switchInt terminator. Best reviewed commit by commit.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Contributor
|
💔 Test for 73d7156 failed: CI. Failed job:
|
Member
Author
|
@bors try jobs=i686-msvc-1,x86_64-msvc-1,x86_64-mingw-1 |
This comment has been minimized.
This comment has been minimized.
rust-bors bot
pushed a commit
that referenced
this pull request
Jan 15, 2026
Only use SSA locals in SimplifyComparisonIntegral try-job: i686-msvc-1 try-job: x86_64-msvc-1 try-job: x86_64-mingw-1
Member
Author
The test that is from #128627 is correct but fragile. The test expects the output is: not: I have dug more into why the The assembly code with .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_27without .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_26IIUC, the This can be verified by changing llvm-mc -filetype=obj -triple=x86_64 dummy_span.s -o dummy_span.o
llvm-dwarfdump --debug-line dummy_span.oThere is some note from https://llvm.org/docs/KeyInstructionsDebugInfo.html:
This tells us:
I think this has explained why LLDB passes the test. 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) |
Contributor
|
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? |
Member
Author
No, just gdb skips |
Contributor
|
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 |
Contributor
Member
Author
|
Member
Author
|
@bors retry (potential flaky test) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #150904.
The place may be modified from the comparison statement to the switchInt terminator.
Best reviewed commit by commit.