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 6 pull requests #118763

Merged
merged 17 commits into from
Dec 9, 2023
Merged

Rollup of 6 pull requests #118763

merged 17 commits into from
Dec 9, 2023

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

Young-Flash and others added 17 commits December 1, 2023 22:17
The instance evaluation is needed to handle intrinsics such as
`type_id` and `type_name`.

Since we now use Allocation to represent all evaluated constants,
provide a few methods to help process the data inside an allocation.
A refactoring in rust-lang#117076 changed the `DefIdVisitorSkeleton` to avoid
calling `visit_projection_ty` for `ty::Projection` aliases, and instead
just iterate over the args - this makes sense, as `visit_projection_ty`
will indirectly visit all of the same args, but in doing so, will also
create a `TraitRef` containing the trait's `DefId`, which also gets
visited. The trait's `DefId` isn't visited when we only visit the
arguments without separating them into `TraitRef` and own args first.

Signed-off-by: David Wood <david@davidtw.co>
- Take more things by self, not &self
- Clone more things
- Rework namespacing so we can use `ty::` in the canonicalizer
… r=lcnr

Uplift the (new solver) canonicalizer into `rustc_next_trait_solver`

Uplifts the new trait solver's canonicalizer into a new crate called `rustc_next_trait_solver`.

The crate name is literally a bikeshed-avoidance name, so let's not block this PR on that -- renames are welcome later.

There are a host of other changes that were required to make this possible:
* Expose a `ConstTy` trait to get the `Interner::Ty` from a `Interner::Const`.
* Expose some constructor methods to construct `Bound` variants. These are currently methods defined on the interner themselves, but they could be pulled into traits later.
* Expose a `IntoKind` trait to turn a `Ty`/`Const`/`Region` into their corresponding `*Kind`s.
* Some minor tweaks to other APIs in `rustc_type_ir`.

The canonicalizer code itself is best reviewed **with whitespace ignored.**

r? ``@lcnr``
fix: correct the arg for 'suggest to use associated function syntax' diagnostic

close rust-lang#118469
Add instance evaluation and methods to read an allocation in StableMIR

The instance evaluation is needed to handle intrinsics such as `type_id` and `type_name`.

Since we now use Allocation to represent all evaluated constants, provide a few methods to help process the data inside an allocation.

I've also started to add a structured way to get information about the compilation target machine. For now, I've only added information needed to process an allocation.

r? ``````@ouz-a``````
…it-trait-ref-and-args, r=TaKO8Ki

privacy: visit trait def id of projections

Fixes rust-lang#117997.

A refactoring in rust-lang#117076 changed the `DefIdVisitorSkeleton` to avoid calling `visit_projection_ty` for `ty::Projection` aliases, and instead just iterate over the args - this makes sense, as `visit_projection_ty` will indirectly visit all of the same args, but in doing so, will also create a `TraitRef` containing the trait's `DefId`, which also gets visited. The trait's `DefId` isn't visited when we only visit the arguments without separating them into `TraitRef` and own args first.

Eventually this influences the reachability set and whether a function is encoded into the metadata.
…er-errors

recurse into refs when comparing tys for diagnostics

before:
![image](https://github.com/rust-lang/rust/assets/23638587/bf6abd62-c7f3-4c09-a47e-31b6e129de19)

after:
![image](https://github.com/rust-lang/rust/assets/23638587/b704d728-ddba-4204-aebe-c07dcbbcb55c)

this diff from the test suite is also quite nice imo:
```diff
`@@` -4,8 +4,8 `@@` error[E0308]: mismatched types
 LL |     debug_assert_eq!(iter.next(), Some(value));
    |                                   ^^^^^^^^^^^ expected `Option<<I as Iterator>::Item>`, found `Option<&<I as Iterator>::Item>`
    |
-   = note: expected enum `Option<<I as Iterator>::Item>`
-              found enum `Option<&<I as Iterator>::Item>`
+   = note: expected enum `Option<_>`
+              found enum `Option<&_>`
```
…mpiler-errors

temporarily revert "ice on ambguity in mir typeck"

Reverts rust-lang#116530 as a temporary measure to fix rust-lang#117577. That issue should be ultimately fixed by checking WF of type annotations prior to normalization, which is implemented in rust-lang#104098 but this PR is intended to be backported to beta.

r? ``@compiler-errors`` (the reviewer of the reverted PR)
@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) rollup A PR which is a rollup labels Dec 8, 2023
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=6

@bors
Copy link
Contributor

bors commented Dec 8, 2023

📌 Commit a255b52 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 Dec 8, 2023
@bors
Copy link
Contributor

bors commented Dec 9, 2023

⌛ Testing commit a255b52 with merge dc3a353...

@bors
Copy link
Contributor

bors commented Dec 9, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 9, 2023
@bors bors merged commit dc3a353 into rust-lang:master Dec 9, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 9, 2023
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#117586 Uplift the (new solver) canonicalizer into `rustc_next_trai… 56f9aff08b57d000df694e7b85b5ee3f16e4898d (link)
#118502 fix: correct the arg for 'suggest to use associated functio… f4753fef2811aa4240ae3ac8b73e864298c67b84 (link)
#118694 Add instance evaluation and methods to read an allocation i… 7d2fe7b68ee6f48d89a42f90b2d91c8fafb23df1 (link)
#118715 privacy: visit trait def id of projections bcb464fcc52084cac1943238fa2bf0415f8ff03b (link)
#118730 recurse into refs when comparing tys for diagnostics 785edeac71cbec78ca54c2a27e36caf2dd7079dd (link)
#118736 temporarily revert "ice on ambguity in mir typeck" 90b8b9d982f91cf9af3ea4c5d4bb983cc7e71090 (link)

previous master: 2d2f1b2099

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

Overall result: ❌ regressions - 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.4% [0.2%, 0.5%] 18
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.2%, 0.5%] 18

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)
- - 0
Improvements ✅
(primary)
-1.8% [-5.3%, -0.5%] 4
Improvements ✅
(secondary)
-3.0% [-3.7%, -2.3%] 2
All ❌✅ (primary) -1.8% [-5.3%, -0.5%] 4

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.9% [-6.0%, -5.7%] 3
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 669.53s -> 669.092s (-0.07%)
Artifact size: 314.04 MiB -> 314.12 MiB (0.02%)

@rustbot rustbot added the perf-regression Performance regression. label Dec 9, 2023
@Kobzol
Copy link
Contributor

Kobzol commented Dec 9, 2023

Hmm, kind of hard to guess for me here what could be the culprit. Let's try something:

@rust-timer build bcb464f

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bcb464f): comparison URL.

Overall result: ❌ regressions - 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)
0.4% [0.2%, 0.5%] 18
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.2%, 0.5%] 18

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.0% [2.5%, 3.6%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.9% [-0.5%, 3.6%] 3

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-6.4% [-6.8%, -6.2%] 3
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 669.53s -> 668.582s (-0.14%)
Artifact size: 314.04 MiB -> 314.07 MiB (0.01%)

@Kobzol
Copy link
Contributor

Kobzol commented Dec 9, 2023

Bingo. @davidtwco were these regressions expected?

@davidtwco
Copy link
Member

Bingo. @davidtwco were these regressions expected?

I wasn't expecting them but they make sense. We do a little bit more work after that PR because the reachable set is slightly bigger and that in turn means more MIR gets encoded and things like that.

@Kobzol
Copy link
Contributor

Kobzol commented Dec 11, 2023

Ok. Marking as triaged then.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 11, 2023
@matthiaskrgr matthiaskrgr deleted the rollup-mgyf5hp branch March 16, 2024 18:19
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. 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.