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 5 pull requests #113911

Merged
merged 34 commits into from
Jul 21, 2023
Merged

Rollup of 5 pull requests #113911

merged 34 commits into from
Jul 21, 2023

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

pcwalton and others added 30 commits July 16, 2023 22:56
…ics.

LLVM has a neat [statistics] feature that tracks how often optimizations kick
in. It's very handy for optimization work. Since we expose the LLVM pass
timings, I thought it made sense to expose the LLVM statistics too.

[statistics]: https://llvm.org/docs/ProgrammersManual.html#the-statistic-class-stats-option
…rust/pull/113723/files

use slice memcpy rather than strcpy and write it on stdout

use println on failure

Co-authored-by: Oli Scherer <github35764891676564198441@oli-obk.de>
This introduces a bit of code duplication, but we don't have the
build_output_filenames in the CrateName arm and this seems a little
cleaner overall.
The style guide discusses the default Rust style. Configurability of
Rust formatting tools are not the domain of the style guide.
The style guide requires a trailing comma on where clause components,
but then gives an example that doesn't include one. Add the missing
trailing comma.
…change)

Avoid putting a sentence fragment after a list; integrate it with the
sentence before the list.
…default style

The style guide inconsistently used language like "there should be a
space" or "it should be on its own line", or "may be written on a single
line", for things that are required components of the default Rust
style. "should" and especially "may" come across as optional. While the
style guide overall now has a statement at the top that the default
style itself is a *recommendation*, the *definition* of the default
style should not be ambiguous about what's part of the default style.

Rewrite language in the style guide to only use "should" and "may" and
similar for truly optional components of the style (e.g. things a tool
cannot or should not enforce in its default configuration).

In their place, either use "must", or rewrite in imperative style ("put
a space", "start it on the same line"). The latter also substantially
reduces the use of passive voice.

This is a purely editorial change, and does not affect the semantic
definition of the Rust style.
Make it clear the rule for stacking the second line on the first applies
recursively, as long as the condition holds.
…rustfmt)

An example immediately following "Put each bound on its own line." did
not put each bound on its own line.
Co-authored-by: Caleb Cartwright <calebcartwright@users.noreply.github.com>
…ust-should-may, r=calebcartwright

style-guide: clean up "must"/"should"/"may"

Avoid using "should" or "may" for required parts of the default style.

The style guide inconsistently used language like "there should be a space" or
"it should be on its own line", or "may be written on a single line", for
things that are required components of the default Rust style. "should" and
especially "may" come across as optional. While the style guide overall now has
a statement at the top that the default style itself is a *recommendation*, the
*definition* of the default style should not be ambiguous about what's part of
the default style.

Rewrite language in the style guide to only use "should" and "may" and similar
for truly optional components of the style (e.g. things a tool cannot or should
not enforce in its default configuration).

In their place, either use "must", or rewrite in imperative style ("put a
space", "start it on the same line"). The latter also substantially reduces the
use of passive voice.

Looking for "should"s also flagged some recommendations the style guide made
for configurability of tools (e.g. a tool "should" have a given configuration
option). I've removed those recommendations, per discussion with the style
team; it's not the domain of the style guide to make such recommendations, only
to define the default Rust style.

In the process of making this change, I also fixed a typo, fixed a text structure
issue, fixed an example that didn't match the Rust style (missing a trailing
comma), and added an additional example for clarity. (Those changes would have
conflicted with this one.) Those changes appear in separate commits.

These are all purely editorial changes, and do not affect the semantic
definition of the Rust style.
…nikic

Resurrect: rustc_llvm: Add a -Z `print-codegen-stats` option to expose LLVM statistics.

This resurrects PR rust-lang#104000, which has sat idle for a while. And I want to see the effect of stack-move optimizations on LLVM (like https://reviews.llvm.org/D153453) :).

I have applied the changes requested by `@oli-obk` and `@nagisa`  rust-lang#104000 (comment) and rust-lang#104000 (comment) in the latest commits.

r? `@oli-obk`

-----

LLVM has a neat [statistics](https://llvm.org/docs/ProgrammersManual.html#the-statistic-class-stats-option) feature that tracks how often optimizations kick in. It's very handy for optimization work. Since we expose the LLVM pass timings, I thought it made sense to expose the LLVM statistics too.

-----
(Edit: fix broken link
(Edit2: fix segmentation fault and use malloc

If `rustc` is built with
```toml
[llvm]
assertions = true
```
Then you can see like
```
rustc +stage1 -Z print-codegen-stats -C opt-level=3  tmp.rs
===-------------------------------------------------------------------------===
                          ... Statistics Collected ...
===-------------------------------------------------------------------------===
         3 aa                           - Number of MayAlias results
       193 aa                           - Number of MustAlias results
       531 aa                           - Number of NoAlias results
...
```

And the current default build emits only
```
$ rustc +stage1 -Z print-codegen-stats -C opt-level=3  tmp.rs
===-------------------------------------------------------------------------===
                          ... Statistics Collected ...
===-------------------------------------------------------------------------===
$
```
This might be better to emit the message to tell assertion flag necessity, but now I can't find how to do that...
Support `--print KIND=PATH` command line syntax

As is already done for `--emit KIND=PATH` and `-L KIND=PATH`.

In the discussion of rust-lang#110785, it was pointed out that `--print KIND=PATH` is nicer than trying to apply the single global `-o` path to `--print`'s output, because in general there can be multiple print requests within a single rustc invocation, and anyway `-o` would already be used for a different meaning in the case of `link-args` and `native-static-libs`.

I am interested in using `--print cfg=PATH` in Buck2. Currently Buck2 works around the lack of support for `--print KIND=PATH` by [indirecting through a Python wrapper script](https://github.com/facebook/buck2/blob/d43cf3a51a31f00be2c2248e78271b0fef0452b4/prelude/rust/tools/get_rustc_cfg.py) to redirect rustc's stdout into the location dictated by the build system.

From skimming Cargo's usages of `--print`, it definitely seems like it would benefit from `--print KIND=PATH` too. Currently it is working around the lack of this by inserting `--crate-name=___ --print=crate-name` so that it can look for a line containing `___` as a delimiter between the 2 other `--print` informations it actually cares about. This is commented as a "HACK" and "abuse". https://github.com/rust-lang/cargo/blob/31eda6f7c360d9911f853b3014e057db61238f3e/src/cargo/core/compiler/build_context/target_info.rs#L242 (FYI `@weihanglo` as you dealt with this recently in rust-lang/cargo#11633.)

Mentioning reviewers active in rust-lang#110785: `@fee1-dead` `@jyn514` `@bjorn3`
Make {Rc,Arc}::allocator associated functions
Minor improvements to Windows TLS dtors

This does a few things:

* Moves keyless dtors into the same module as the `on_tls_callback` function because of dylib mess. We keep the `inline(never)` hints as a precaution (see also the issue they link to).
* Introduces the `HAS_DTORS` atomic as an optimization hint. This allows removing (most) of the TLS dtor code if no dtors are ever run. Otherwise it's always included because of a `#[used]`.
* Only run either keyed dtors or keyless dtors but not both. They should be mutually exclusive as keyed dtors are a fallback. I've also added an `assert` to make sure this is true.
@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-style Relevant to the style team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Jul 21, 2023
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=6

@bors
Copy link
Contributor

bors commented Jul 21, 2023

📌 Commit 8c6ef1d has been approved by matthiaskrgr

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 Jul 21, 2023
@bors
Copy link
Contributor

bors commented Jul 21, 2023

⌛ Testing commit 8c6ef1d with merge 78f97c9...

@bors
Copy link
Contributor

bors commented Jul 21, 2023

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing 78f97c9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 21, 2023
@bors bors merged commit 78f97c9 into rust-lang:master Jul 21, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 21, 2023
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#113380 style-guide: clean up "must"/"should"/"may" c6f34cf6816022d4fbaf7c0e4492955c6c606e01 (link)
#113723 Resurrect: rustc_llvm: Add a -Z print-codegen-stats optio… 073bed5ef67415a894f0b63a1c6f0093b33c4557 (link)
#113780 Support --print KIND=PATH command line syntax ee20ad0513204e09d11d523bbe0655887984c745 (link)
#113810 Make {Rc,Arc}::allocator associated functions 29b2769c27d4c256c5de1a8f7d373da9456f94e8 (link)
#113907 Minor improvements to Windows TLS dtors 5ca95a73694cbe029c4fdc84e540aa90e778c31a (link)

previous master: 1a44b45987

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

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (78f97c9): 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

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.2% [3.2%, 3.2%] 1
Regressions ❌
(secondary)
1.4% [1.4%, 1.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.2% [3.2%, 3.2%] 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)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 1

Binary size

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

Bootstrap: 650.086s -> 651.312s (0.19%)

@matthiaskrgr matthiaskrgr deleted the rollup-wk6cr7v branch March 16, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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-style Relevant to the style 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