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 #131581

Merged
merged 18 commits into from
Oct 12, 2024
Merged

Rollup of 7 pull requests #131581

merged 18 commits into from
Oct 12, 2024

Conversation

tgross35
Copy link
Contributor

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

taiki-e and others added 18 commits October 6, 2024 08:14
Add intrinsics `fmuladd{f16,f32,f64,f128}`. This computes `(a * b) +
c`, to be fused if the code generator determines that (i) the target
instruction set has support for a fused operation, and (ii) that the
fused operation is more efficient than the equivalent, separate pair
of `mul` and `add` instructions.

https://llvm.org/docs/LangRef.html#llvm-fmuladd-intrinsic

MIRI support is included for f32 and f64.

The codegen_cranelift uses the `fma` function from libc, which is a
correct implementation, but without the desired performance semantic. I
think this requires an update to cranelift to expose a suitable
instruction in its IR.

I have not tested with codegen_gcc, but it should behave the same
way (using `fma` from libc).
For the expr with attributes, like `let _ = (#[inline] || println!("Hello!"));`, the suggestion's span should contains the attributes, or the suggestion will remove them.

fixes rust-lang#129833
…thlin

intrinsics fmuladdf{32,64}: expose llvm.fmuladd.* semantics

Add intrinsics `fmuladd{f32,f64}`. This computes `(a * b) + c`, to be fused if the code generator determines that (i) the target instruction set has support for a fused operation, and (ii) that the fused operation is more efficient than the equivalent, separate pair of `mul` and `add` instructions.

https://llvm.org/docs/LangRef.html#llvm-fmuladd-intrinsic

The codegen_cranelift uses the `fma` function from libc, which is a correct implementation, but without the desired performance semantic. I think this requires an update to cranelift to expose a suitable instruction in its IR.

I have not tested with codegen_gcc, but it should behave the same way (using `fma` from libc).

---
This topic has been discussed a few times on Zulip and was suggested, for example, by `@workingjubilee` in [Effect of fma disabled](https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/Effect.20of.20fma.20disabled/near/274179331).
Migrate lib's `&Option<T>` into `Option<&T>`

Trying out my new lint rust-lang/rust-clippy#13336 - according to the [video](https://www.youtube.com/watch?v=6c7pZYP_iIE), this could lead to some performance and memory optimizations.

Basic thoughts expressed in the video that seem to make sense:
* `&Option<T>` in an API breaks encapsulation:
  * caller must own T and move it into an Option to call with it
  * if returned, the owner must store it as Option<T> internally in order to return it
* Performance is subject to compiler optimization, but at the basics, `&Option<T>` points to memory that has `presence` flag + value, whereas `Option<&T>` by specification is always optimized to a single pointer.
…tgross35

stabilize duration_consts_float

Waiting for FCP in rust-lang#72440 to pass.

`as_millis_f32` and `as_millis_f64` are not stable at all yet, so I moved their const-stability together with their regular stability (tracked at rust-lang#122451).

Fixes rust-lang#72440
Support clobber_abi in MSP430 inline assembly

This supports `clobber_abi` which is one of the requirements of stabilization mentioned in rust-lang#93335.

Refs: Section 3.2 "Register Conventions" in [MSP430 Embedded Application Binary Interface](https://www.ti.com/lit/an/slaa534a/slaa534a.pdf)

cc ``@cr1901``

r? ``@Amanieu``

``@rustbot`` label +O-msp430
Make unused_parens's suggestion considering expr's attributes.

For the expr with attributes,
like `let _ = (#[inline] || println!("Hello!"));`,
the suggestion's span should contains the attributes, or the suggestion will remove them.

fixes rust-lang#129833
…r=compiler-errors

Remove deprecation note in the `non_local_definitions` lint

This PR removes the edition deprecation note emitted by the `non_local_definitions` lint.

Specifically this part:

```
= note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue <rust-lang#120363>
```

because it [didn't make the cut](rust-lang#120363 (comment)) for the 2024 edition.

`@rustbot` label +L-non_local_definitions
Flatten redundant test module `run_make_support::diff::tests::tests`

This module is already named `tests`, and is already gated by `#[cfg(test)]`, so there's no need for it to also contain `mod tests`.

r? jieyouxu
@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs O-SGX Target: SGX O-unix Operating system: Unix-like 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 Oct 12, 2024
@tgross35
Copy link
Contributor Author

@bors r+ rollup=never p=7

@bors
Copy link
Contributor

bors commented Oct 12, 2024

📌 Commit 5e477c9 has been approved by tgross35

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

bors commented Oct 12, 2024

⌛ Testing commit 5e477c9 with merge 7b6e8c9...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#124874 (intrinsics fmuladdf{32,64}: expose llvm.fmuladd.* semantics)
 - rust-lang#130962 (Migrate lib's `&Option<T>` into `Option<&T>`)
 - rust-lang#131289 (stabilize duration_consts_float)
 - rust-lang#131310 (Support clobber_abi in MSP430 inline assembly)
 - rust-lang#131546 (Make unused_parens's suggestion considering expr's attributes.)
 - rust-lang#131565 (Remove deprecation note in the `non_local_definitions` lint)
 - rust-lang#131576 (Flatten redundant test module `run_make_support::diff::tests::tests`)

r? `@ghost`
`@rustbot` modify labels: rollup
@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] miri test:false 5.617
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Sat, Oct 12, 2024  5:40:29 AM
  network time: Sat, 12 Oct 2024 05:40:29 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@bors
Copy link
Contributor

bors commented Oct 12, 2024

💔 Test failed - checks-actions

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

MSVC failure

@bors retry

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

bors commented Oct 12, 2024

⌛ Testing commit 5e477c9 with merge 8f8bee4...

@bors
Copy link
Contributor

bors commented Oct 12, 2024

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing 8f8bee4 to master...

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

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#124874 intrinsics fmuladdf{32,64}: expose llvm.fmuladd.* semantics b47effc3e708a7107e88dc9c5d41f02f6319bf79 (link)
#130962 Migrate lib's &Option<T> into Option<&T> e98d71be1e3cd7f32c94af6893390ae8825bd77a (link)
#131289 stabilize duration_consts_float 61dc601e238dba12f14fd958915efe7b27fa4bef (link)
#131310 Support clobber_abi in MSP430 inline assembly c5c502edf66e23072102f19ae56bb7a3f2638b31 (link)
#131546 Make unused_parens's suggestion considering expr's attribut… 28786e1da31f133991898d94502ca69911808ef2 (link)
#131565 Remove deprecation note in the non_local_definitions lint e543b141bb75863caa381de19cb4fa398b2c2346 (link)
#131576 Flatten redundant test module `run_make_support::diff::test… 3fd9888c8976a75af3fb670ce284f543dba07853 (link)

previous master: fb20e4d3b9

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 (8f8bee4): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.4% [0.3%, 0.6%] 6
Improvements ✅
(primary)
-0.3% [-1.0%, -0.2%] 7
Improvements ✅
(secondary)
-2.5% [-2.5%, -2.5%] 1
All ❌✅ (primary) -0.3% [-1.0%, -0.2%] 7

Max RSS (memory usage)

Results (primary 1.9%, secondary -2.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.

mean range count
Regressions ❌
(primary)
1.9% [1.2%, 2.7%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.7% [-3.7%, -1.4%] 3
All ❌✅ (primary) 1.9% [1.2%, 2.7%] 2

Cycles

Results (secondary 7.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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
7.7% [5.4%, 9.1%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 781.192s -> 779.421s (-0.23%)
Artifact size: 332.04 MiB -> 332.13 MiB (0.03%)

@rustbot rustbot added the perf-regression Performance regression. label Oct 12, 2024
@tgross35 tgross35 deleted the rollup-vul4kol branch October 12, 2024 15:56
@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs merged-by-bors This PR was explicitly merged by bors. O-SGX Target: SGX O-unix Operating system: Unix-like perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.