Skip to content

Revert "Apply EarlyOtherwiseBranch to scalar value #129047" - #130775

Merged
bors merged 2 commits into
rust-lang:masterfrom
jieyouxu:revert-129047
Sep 24, 2024
Merged

Revert "Apply EarlyOtherwiseBranch to scalar value #129047"#130775
bors merged 2 commits into
rust-lang:masterfrom
jieyouxu:revert-129047

Conversation

@jieyouxu

@jieyouxujieyouxu commented Sep 24, 2024

Copy link
Copy Markdown
Member

This reverts PR #129047, commit a772336, reversing changes made to 702987f.

cc @dianqk and @cjgillot as the PR author and reviewer of #129047 respectively.

It seems Apply EarlyOtherwiseBranch to scalar value #129047 may have lead to several nightly regressions:

Example test that would ICE with changes in #129047 (this test is included in this PR):

//@ compile-flags: -C opt-level=3//@ check-passuse std::task::Poll;pubfnpoll(val:Poll<Result<Option<Vec<u8>>,u8>>){match val {Poll::Ready(Ok(Some(_trailers))) => {}Poll::Ready(Err(_err)) => {}Poll::Ready(Ok(None)) => {}Poll::Pending => {}}}

Since this is a mir-opt ICE that seems to quite easy to trigger with real-world crates being affected, let's revert for now and reland the mir-opt after these are fixed.

…h_scalar, r=cjgillot"
This reverts commit a772336, reversing
changes made to 702987f.
It seems Apply EarlyOtherwiseBranch to scalar value rust-lang#129047 may have
lead to several nightly regressions:
- rust-lang#130769
- rust-lang#130774
- rust-lang#130771
And since this is a mir-opt ICE that seems to quite easy to trigger with
real-world crates being affected, let's revert for now and reland the
mir-opt later.
@rustbot

Copy link
Copy Markdown
Collaborator

r? @chenyukang

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

@rustbotrustbot 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 Sep 24, 2024
@rustbot

Copy link
Copy Markdown
Collaborator

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@chenyukang

Copy link
Copy Markdown
Member

r? @rust-lang/wg-mir-opt

@rustbotrustbot assigned davidtwco and unassigned chenyukangSep 24, 2024
@fmease

Copy link
Copy Markdown
Member

Could you perhaps add a regression test (in a separate commit)?

@jieyouxu

Copy link
Copy Markdown
MemberAuthor

Could you perhaps add a regression test (in a separate commit)?

(I did just before you asked lol)

@nagisa

Copy link
Copy Markdown
Member

@bors r+ p=1

Fixing a critical regression, should land ASAP.

@bors

bors commented Sep 24, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit ad7eb48 has been approved by nagisa

It is now in the queue for this repository.

@borsbors 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 Sep 24, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 24, 2024
Revert "Apply EarlyOtherwiseBranch to scalar value rust-lang#129047"
This reverts PR rust-lang#129047, commit a772336, reversing changes made to 702987f.
cc `@DianQK` and `@cjgillot` as the PR author and reviewer of rust-lang#129047 respectively.
It seems [Apply EarlyOtherwiseBranch to scalar value rust-lang#129047](rust-lang#129047) may have lead to several nightly regressions:
- rust-lang#130769
- rust-lang#130774
- rust-lang#130771
Example test that would ICE with changes in rust-lang#129047 (this test is included in this PR):
```rs
//@ compile-flags: -C opt-level=3
//@ check-pass
use std::task::Poll;
pub fn poll(val: Poll<Result<Option<Vec<u8>>, u8>>) {
match val {
Poll::Ready(Ok(Some(_trailers))) => {}
Poll::Ready(Err(_err)) => {}
Poll::Ready(Ok(None)) => {}
Poll::Pending => {}
}
}
```
Since this is a mir-opt ICE that seems to quite easy to trigger with real-world crates being affected, let's revert for now and reland the mir-opt after these are fixed.
@bors

bors commented Sep 24, 2024

Copy link
Copy Markdown
Collaborator

⌛ Testing commit ad7eb48 with merge 3b262d8...

@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

The job x86_64-gnu-aux failed! Check out the build log: (web)(plain)

Click to see the possible cause of the failure (guessed by this bot)
warning: spurious network error (1 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
warning: spurious network error (1 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
warning: spurious network error (1 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
warning: spurious network error (1 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
error: failed to get `half` as a dependency of package `ciborium-ll v0.2.2`
... which satisfies dependency `ciborium-ll = "^0.2.2"` (locked to 0.2.2) of package `ciborium v0.2.2`
... which satisfies dependency `ciborium = "^0.2.0"` (locked to 0.2.2) of package `criterion v0.5.1`
... which satisfies dependency `criterion = "^0.5.1"` (locked to 0.5.1) of package `benchsuite v0.0.0 (/checkout/src/tools/cargo/benches/benchsuite)`
Caused by:
Caused by:
download of ha/lf/half failed
Caused by:
Caused by:
failed to download from `https://index.crates.io/ha/lf/half`
Caused by:
[28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
Build completed unsuccessfully in 0:26:32
Build completed unsuccessfully in 0:26:32
make: *** [Makefile:51: check-aux] Error 1
network time: Tue, 24 Sep 2024 12:01:13 GMT
##[error]Process completed with exit code 2.
Post job cleanup.

@bors

bors commented Sep 24, 2024

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@borsbors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 24, 2024
@jieyouxu

Copy link
Copy Markdown
MemberAuthor

@bors retry (spurious network error)

@borsbors 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 Sep 24, 2024
@bors

bors commented Sep 24, 2024

Copy link
Copy Markdown
Collaborator

⌛ Testing commit ad7eb48 with merge 67bb749...

@bors

bors commented Sep 24, 2024

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 67bb749 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Sep 24, 2024
@bors
bors merged commit 67bb749 into rust-lang:masterSep 24, 2024
@rustbotrustbot added this to the 1.83.0 milestone Sep 24, 2024
@jieyouxu
jieyouxu deleted the revert-129047 branch September 24, 2024 17:54
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (67bb749): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)
0.6%[0.6%, 0.6%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-1.1%[-1.1%, -1.1%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)-0.2%[-1.1%, 0.6%]2

Max RSS (memory usage)

Results (primary -4.0%, secondary 3.7%)

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.

meanrangecount
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
3.7%[3.6%, 3.8%]2
Improvements ✅
(primary)
-4.0%[-6.6%, -1.5%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)-4.0%[-6.6%, -1.5%]2

Cycles

Results (primary -0.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.

meanrangecount
Regressions ❌
(primary)
0.6%[0.5%, 0.7%]2
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-1.9%[-2.1%, -1.7%]2
Improvements ✅
(secondary)
--0
All ❌✅ (primary)-0.6%[-2.1%, 0.7%]4

Binary size

Results (primary -0.4%, secondary -0.0%)

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.

meanrangecount
Regressions ❌
(primary)
0.1%[0.1%, 0.1%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-0.5%[-0.8%, -0.1%]3
Improvements ✅
(secondary)
-0.0%[-0.0%, -0.0%]1
All ❌✅ (primary)-0.4%[-0.8%, 0.1%]4

Bootstrap: 769.56s -> 767.328s (-0.29%)
Artifact size: 340.89 MiB -> 340.88 MiB (-0.00%)

@rustbotrustbot added the perf-regression Performance regression. label Sep 24, 2024
@lqd

lqd commented Sep 24, 2024

Copy link
Copy Markdown
Member

This is a revert, no investigation needed.

@rustbot label: +perf-regression-triaged

@rustbotrustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 24, 2024
github-merge-queueBot pushed a commit to model-checking/kani that referenced this pull request Sep 25, 2024
Update Rust toolchain from nightly-2024-09-23 to nightly-2024-09-25
without any other source changes.
We skip over 2024-09-24 as that version ICEs when trying to build
http-body (fixed by rust-lang/rust#130775).
---------
Co-authored-by: celinval <35149715+celinval@users.noreply.github.com>
Co-authored-by: Michael Tautschnig <tautschn@amazon.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-borsThis PR was explicitly merged by bors.perf-regressionPerformance regression.perf-regression-triagedThe performance regression has been triaged.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant 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.

10 participants

@jieyouxu@rustbot@chenyukang@fmease@nagisa@bors@rust-log-analyzer@rust-timer@lqd@davidtwco