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 10 pull requests #88857

Merged
merged 22 commits into from
Sep 11, 2021
Merged

Conversation

workingjubilee
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

kpreid and others added 22 commits August 9, 2021 18:55
The existing documentation felt a little unhelpfully concise, so this
change tries to improve it by using longer sentences, each of which
specifies which kinds of types it applies to as early as possible. In
particular, the third item starts with “Structs ...” instead of
saying “Foo is a struct” later.

Also, the previous list items “Only the last field has a type
involving `T`” and “`T` is not part of the type of any other
fields” are, as far as I see, redundant with each other, so I removed
the latter.
Commit 95e096d changed a bunch of size checks already, but more have
been added, so this fixes the new ones the same way: the various size
checks that are conditional on target_arch = "x86_64" were not intended
to apply to x86_64-unknown-linux-gnux32, so add
target_pointer_width = "64" to the conditions.
example: let x: String = String::new().into();
Reword description of automatic impls of `Unsize`.

The existing documentation felt a little unhelpfully concise, so this change tries to improve it by using longer sentences, each of which specifies which kinds of types it applies to as early as possible. In particular, the third item starts with “Structs ...” instead of saying “Foo is a struct” later.

Also, the previous list items “Only the last field has a type involving `T`” and “`T` is not part of the type of any other fields” are, as far as I see, redundant with each other, so I removed the latter.

I have no particular knowledge of `Unsize`; I have attempted to leave the meaning entirely unchanged but may have missed a nuance.

Markdown preview of the edited documentation:

> All implementations of `Unsize` are provided automatically by the compiler.
> Those implementations are:
>
> - Arrays `[T; N]` implement `Unsize<[T]>`.
> - Types implementing a trait `Trait` also implement `Unsize<dyn Trait>`.
> - Structs `Foo<..., T, ...>` implement `Unsize<Foo<..., U, ...>>` if all of these conditions
>   are met:
>   - `T: Unsize<U>`.
>   - Only the last field of `Foo` has a type involving `T`.
>   - `Bar<T>: Unsize<Bar<U>>`, where `Bar<T>` stands for the actual type of that last field.
Fix non-capturing closure return type coercion

Fixes rust-lang#88097. For the example given there:
```rust
fn peculiar() -> impl Fn(u8) -> u8 {
    return |x| x + 1
}
```
which incorrectly reports an error, I noticed something weird in the debug log:
```
DEBUG rustc_typeck::check::coercion coercion::try_find_coercion_lub([closure@test.rs:2:12: 2:21], [closure@test.rs:2:12: 2:21], exprs=1 exprs)
```
Apparently, `try_find_coercion_lub()` thinks that the LUB for two closure types always has to be a function pointer (which explains the `expected closure, found fn pointer` error in rust-lang#88097). There is one corner case where that isn't true, though — namely, when the two closure types are equal, in which case the trivial LUB is the type itself. This PR fixes this by inserting an explicit check for type equality in `try_find_coercion_lub()`.
Improve error message when _ is used for in/inout asm operands

As suggested by ```@Commeownist``` in rust-lang#72016 (comment).
Change more x64 size checks to not apply to x32.

Commit 95e096d changed a bunch of size checks already, but more have
been added, so this fixes the new ones the same way: the various size
checks that are conditional on target_arch = "x86_64" were not intended
to apply to x86_64-unknown-linux-gnux32, so add
target_pointer_width = "64" to the conditions.
Fix ICE for functions with more than 65535 arguments

This pull request fixes rust-lang#88577 by changing the `param_idx` field in the `Param` variant of `WellFormedLoc` from `u16` to `u32`, thus allowing for more than 65,535 arguments in a function. Note that I also added a regression test, but needed to add `// ignore-tidy-filelength` because the test is more than 8000 lines long.
Suggest wapping expr in parentheses on invalid unary negation

Fixes rust-lang#88701
Use more accurate spans for "unused delimiter" lint
…chenkov

don't clone types that are Copy (clippy::clone_on_copy)
…h726

don't convert types into identical types

example: let x: String = String::new().into();
@rustbot rustbot added the rollup A PR which is a rollup label Sep 11, 2021
@workingjubilee
Copy link
Member Author

@bors: r+ p=11 rollup=never

@bors
Copy link
Contributor

bors commented Sep 11, 2021

📌 Commit 2a8ad06 has been approved by workingjubilee

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 11, 2021
@workingjubilee
Copy link
Member Author

Omitted PRs that seemed like they might affect exports from std::os somehow.
No obvious reason for why that would be the case, but that's the only clue to go on for the last rollup's failure.

@bors
Copy link
Contributor

bors commented Sep 11, 2021

⌛ Testing commit 2a8ad06 with merge 43769af...

@bors
Copy link
Contributor

bors commented Sep 11, 2021

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 43769af to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 11, 2021
@bors bors merged commit 43769af into rust-lang:master Sep 11, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 11, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (43769af): comparison url.

Summary: This change led to very large relevant improvements 🎉 in compiler performance.

  • Very large improvement in instruction counts (up to -45.0% on full builds of coercions)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@workingjubilee workingjubilee deleted the rollup-zrtvspt branch September 12, 2021 08:25
@Mark-Simulacrum
Copy link
Member

This is very surprising to me, given the list of benchmarks in this PR -- do we have a suspect for this?

@jackh726 do you think #88147 may be responsible here? If so, it seems a little surprising -- I'd expect that the rather small change there wouldn't be responsible (or is perhaps subtly wrong or something?). Maybe this is just undiscovered low hanging fruit though!

@jackh726
Copy link
Member

Woah. Uh, maybe...

I would be surprised if #88147 is wrong, given that the LUB of two equal types is itself. I guess it's not surprising that we can be much faster in the easy case; I think before we would have done a bunch of work that we didn't really need to do. But I think maybe I might poke @nikomatsakis here.

@jackh726
Copy link
Member

The perf certainly are nothing to scoff at...

@workingjubilee
Copy link
Member Author

We could post the reversed PR and bench-compare that, see if we observe a similar difference.

@workingjubilee
Copy link
Member Author

I ran that experiment in #88926 and we can see decisively it is indeed that change: https://perf.rust-lang.org/compare.html?start=9f85cd6f2ab2769c16e89dcdddb3e11d9736b351&end=cc08b15420582202a8bffdb12043459d014ffb75

I am not surprised at the result having a massive improvement, though. The fastest code is the code that doesn't run. An occasional branch, costing a few bytes and cycles, especially a predictable one (so there's no pipeline stalls from a mispredict) will in fact yield a massive win on a test for something in that niche.

@Mark-Simulacrum
Copy link
Member

Thanks @workingjubilee!

Yeah, the big win is not that surprising here, it's more so that I was worried we hadn't previously done it for some good reason (e.g., side effects in the code below that new if). But it seems like that's not the case.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.