Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable jump threading UnOp::Not for non-bool #131201

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

compiler-errors
Copy link
Member

Fix #131195, where jumpthreading was optimizing !a == b into a != b for non-bool, where this is definitely not true.

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
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 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 Oct 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@compiler-errors
Copy link
Member Author

I would not be surprised if jumpthreading has more type-based bugs, since we've already found two at this point.

@compiler-errors compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 3, 2024
@compiler-errors
Copy link
Member Author

#131203

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 3, 2024
…piler-errors

JumpThreading: fix bitwise not on non-booleans

Fixes rust-lang#131195

Alternative to rust-lang#131201
@compiler-errors compiler-errors added the stable-nominated Nominated for backporting to the compiler in the stable channel. label Oct 3, 2024
@compiler-errors
Copy link
Member Author

compiler-errors commented Oct 3, 2024

This is a more conservative fix than #131203, which seems like it still has problems.

@cjgillot
Copy link
Contributor

cjgillot commented Oct 4, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2024

📌 Commit f0bfba2 has been approved by cjgillot

It is now in the queue for this repository.

@bors 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 Oct 4, 2024
@bors
Copy link
Contributor

bors commented Oct 4, 2024

⌛ Testing commit f0bfba2 with merge 11ee3a8...

@bors
Copy link
Contributor

bors commented Oct 4, 2024

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 11ee3a8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 4, 2024
@bors bors merged commit 11ee3a8 into rust-lang:master Oct 4, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 4, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (11ee3a8): 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 -4.2%)

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)
-4.2% [-5.3%, -3.0%] 2
All ❌✅ (primary) - - 0

Cycles

Results (secondary 2.6%)

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.6% [2.1%, 2.8%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 772.19s -> 772.334s (0.02%)
Artifact size: 342.02 MiB -> 342.01 MiB (-0.00%)

@lqd
Copy link
Member

lqd commented Oct 11, 2024

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

Stable nomination is also accepted in principle but it's not really needed: the new stable should have this beta backport when the stable artifacts are built in 3 days. Therefore, I'll remove the stable nomination, as it's a no-op this late in the cycle.

@rustbot label +beta-accepted -stable-nominated

@rustbot rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed stable-nominated Nominated for backporting to the compiler in the stable channel. labels Oct 11, 2024
@cuviper cuviper mentioned this pull request Oct 11, 2024
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 11, 2024
@cuviper cuviper modified the milestones: 1.83.0, 1.82.0 Oct 11, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2024
[beta] backports

- Only add an automatic SONAME for Rust dylibs rust-lang#130960
- Reject leading unsafe in `cfg!(...)` and `--check-cfg` rust-lang#131057, resolving rust-lang#131055
- Disable jump threading `UnOp::Not` for non-bool rust-lang#131201
- Update LLVM submodule rust-lang#131448

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2024
[beta] backports

- Only add an automatic SONAME for Rust dylibs rust-lang#130960
- Reject leading unsafe in `cfg!(...)` and `--check-cfg` rust-lang#131057, resolving rust-lang#131055
- Disable jump threading `UnOp::Not` for non-bool rust-lang#131201
- Update LLVM submodule rust-lang#131448

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2024
[beta] backports

- Only add an automatic SONAME for Rust dylibs rust-lang#130960
- Reject leading unsafe in `cfg!(...)` and `--check-cfg` rust-lang#131057, resolving rust-lang#131055
- Disable jump threading `UnOp::Not` for non-bool rust-lang#131201
- Update LLVM submodule rust-lang#131448

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2024
[beta] backports

- Only add an automatic SONAME for Rust dylibs rust-lang#130960
- Reject leading unsafe in `cfg!(...)` and `--check-cfg` rust-lang#131057, resolving rust-lang#131055
- Disable jump threading `UnOp::Not` for non-bool rust-lang#131201
- Update LLVM submodule rust-lang#131448
- Split x86_64-msvc-ext into two jobs rust-lang#130072
- Use a small runner for msvc-ext2 job rust-lang#130151

r? ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-Zmir-opt-level miscompiles code with panic
8 participants