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

Rollup of 7 pull requests #121804

Merged
merged 43 commits into from
Feb 29, 2024
Merged

Rollup of 7 pull requests #121804

merged 43 commits into from
Feb 29, 2024

Conversation

GuillaumeGomez
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

CKingX and others added 30 commits February 8, 2024 17:15
…ers and Rust plans to set Windows 10 as the minimum supported OS for target x86_64-pc-windows-msvc, I have added the cmpxchg16b and sse3 feature (as CPUs that meet the Windows 10 64-bit requirement also support SSE3. See https://walbourn.github.io/directxmath-sse3-and-ssse3/ )
Fixed a bug where adding CMPXCHG16B would fail due to different names in Rustc and LLVM
As CMPXCHG16B is supported, I updated the max atomic width to 128-bits from 64-bits
Updated x86_64-uwp-windows-gnu to use CMPXCHG16B and SSE3
This gives one extra error message on one test, but is necessary to fix
bigger problems caused by the cancellation of stashed errors.

(Note: why not just avoid stashing altogether? Because that resulted in
additional output changes.)
This gives one extra error message on two tests, but is necessary to fix
bigger problems caused by the cancellation of stashed errors.

(Note: why not just avoid stashing altogether? Because that resulted in
additional output changes.)
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. rust-lang#120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.

This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
  errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
  disallowing the use of `steal_diagnostic` with errors, and introducing
  the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
  that can be used instead.

Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
  return `Option<ErrorGuaranteed>`, which enables the removal of two
  `delayed_bug` calls and one `Ty::new_error_with_message` call. This is
  possible because we store error guarantees in
  `DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
  alongside calls to `has_errors`, which is a nice simplification, and
  eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
  error message.

Fixes rust-lang#121451.
Fixes rust-lang#121477.
Fixes rust-lang#121504.
Fixes rust-lang#121508.
This undoes the changes from rust-lang#121482 and rust-lang#121586, now that stashed errors
will trigger more early aborts.
Seems wise, since it shouldn't proceed in that case.
I removed it in rust-lang#121206 because I thought thought it wasn't necessary.
But then I had to add an `emit_stashed_diagnostics` call elsewhere in
rustfmt to avoid the assertion failure (which took two attempts to get
right, rust-lang#121487 and rust-lang#121615), and now there's an assertion failure in
clippy as well (rust-lang/rust-clippy#12364).

So this commit just reinstates the call in `DiagCtxtInner::drop`. It
also reverts the rustfmt changes from rust-lang#121487 and rust-lang#121615, though it
keeps the tests added for those PRs.
In practice, 'a and 'b and 'c are always the same. This change makes
`UnusedExterns` more like `ArtifactNotification`, which uses a single
lifetime 'a in multiple ways.
This `HumanEmitter` is only created to test if it supports colour. The
diagnostic width isn't relevant.
It only has two call sites, and one of those doesn't set the source map.
Because it's now the only constructor.
Use volatile access instead of `#[used]` for `on_tls_callback`

The first commit adds a volatile load of `p_thread_callback` when registering a dtor so that the compiler knows if the callback is used or not. I don't believe the added volatile instruction is otherwise significant in the context. In my testing using the volatile load allowed the compiler to correctly reason about whether `on_tls_callback` is used or not, allowing it to be omitted entirely in some cases. Admittedly it usually is used due to `Thread` but that can be avoided (e.g. in DLLs or with custom entry points that avoid the offending APIs). Ideally this would be something the compiler could help a bit more with so we didn't have to use tricks like `#[used]` or volatile. But alas.

I also used this as an opportunity to clean up the `unused` lints which I don't think serve a purpose any more.

The second commit removes the volatile load of `_tls_used` with `#cfg[target_thread_local]` because `#[thread_local]` already implies it. And if it ever didn't then `#[thread_local]` would be broken when there aren't any dtors.
…in, r=estebank

Count stashed errors again

Stashed diagnostics are such a pain. Their "might be emitted, might not" semantics messes with lots of things.

rust-lang#120828 and rust-lang#121206 made some big changes to how they work, improving some things, but still leaving some problems, as seen by the issues caused by rust-lang#121206. This PR aims to fix all of them by restricting them in a way that eliminates the "might be emitted, might not" semantics while still allowing 98% of their benefit. Details in the individual commit logs.

r? `@oli-obk`
…i-obk

Emitter cleanups

Some cleanups I made when reading emitter code. In particular, `HumanEmitter` and `JsonEmitter` have gone from three constructors to one.

r? `@oli-obk`
@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) O-windows Operating system: Windows 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Feb 29, 2024
@GuillaumeGomez
Copy link
Member Author

@bors r+ p=5 rollup=never

@bors
Copy link
Contributor

bors commented Feb 29, 2024

📌 Commit 0e9f02d has been approved by GuillaumeGomez

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 Feb 29, 2024
@bors
Copy link
Contributor

bors commented Feb 29, 2024

⌛ Testing commit 0e9f02d with merge 1a1876c...

@klensy
Copy link
Contributor

klensy commented Feb 29, 2024

First PR missed again.

@GuillaumeGomez
Copy link
Member Author

Sounds like a very weird bors bug...

@klensy
Copy link
Contributor

klensy commented Feb 29, 2024

Maybe reapprove that PR?

@bors
Copy link
Contributor

bors commented Feb 29, 2024

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 1a1876c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 29, 2024
@bors bors merged commit 1a1876c into rust-lang:master Feb 29, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 29, 2024
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#120820 Enable CMPXCHG16B, SSE3, SAHF/LAHF and 128-bit Atomics (in … 8bbe83cebd35c249309cff167f4a37516933f076 (link)
#121000 pattern_analysis: rework how we hide empty private fields b5ae3af169477ea330f0f52dfc960928adf38b15 (link)
#121376 Skip unnecessary comparison with half-open range patterns 64fe0d8980c130913a45fdd5b2572ac2e3c070c9 (link)
#121596 Use volatile access instead of #[used] for `on_tls_callba… b2d35934b638137cfdf18ad49c37bfee95dc9c46 (link)
#121669 Count stashed errors again 0cb66bd21d71f58a2bcf4a2c41653ac889096d9e (link)
#121783 Emitter cleanups 74932d3031527038d39e5a0cbf2bde7f151bfaeb (link)

previous master: 384d26fc7e

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@bors bors mentioned this pull request Feb 29, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1a1876c): 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.

mean range count
Regressions ❌
(primary)
0.3% [0.2%, 0.6%] 7
Regressions ❌
(secondary)
1.8% [1.7%, 1.9%] 6
Improvements ✅
(primary)
-0.8% [-0.9%, -0.8%] 4
Improvements ✅
(secondary)
-0.5% [-0.8%, -0.2%] 8
All ❌✅ (primary) -0.1% [-0.9%, 0.6%] 11

Max RSS (memory usage)

Results

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

Cycles

Results

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)
4.9% [4.2%, 5.5%] 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: 651.155s -> 650.957s (-0.03%)
Artifact size: 311.15 MiB -> 311.09 MiB (-0.02%)

@rustbot rustbot added the perf-regression Performance regression. label Feb 29, 2024
@GuillaumeGomez GuillaumeGomez deleted the rollup-jh0v3ex branch February 29, 2024 22:36
@Mark-Simulacrum
Copy link
Member

@rust-timer build b5ae3af

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b5ae3af): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

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

mean range count
Regressions ❌
(primary)
3.8% [3.8%, 3.8%] 1
Regressions ❌
(secondary)
1.8% [1.7%, 1.9%] 6
Improvements ✅
(primary)
-0.8% [-0.8%, -0.7%] 4
Improvements ✅
(secondary)
-0.4% [-0.6%, -0.2%] 7
All ❌✅ (primary) 0.1% [-0.8%, 3.8%] 5

Max RSS (memory usage)

Results

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)
2.1% [2.1%, 2.1%] 1
Regressions ❌
(secondary)
2.1% [1.3%, 3.1%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.1% [2.1%, 2.1%] 1

Cycles

Results

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)
3.9% [3.9%, 3.9%] 1
Regressions ❌
(secondary)
4.6% [4.1%, 5.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.8% [-3.8%, -3.8%] 1
All ❌✅ (primary) 3.9% [3.9%, 3.9%] 1

Binary size

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

Bootstrap: 651.155s -> 651.22s (0.01%)
Artifact size: 311.15 MiB -> 311.07 MiB (-0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) merged-by-bors This PR was explicitly merged by bors. O-windows Operating system: Windows perf-regression Performance regression. rollup A PR which is a rollup 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc 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