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.

Sign up for GitHub

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

Jump to bottom

Store the types of ty::Expr arguments in the ty::Expr #125968

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Store the types of ty::Expr arguments in the ty::Expr #125968

merged 2 commits into from
Jun 4, 2024

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Jun 4, 2024

Part of #125958

In attempting to remove the ty field on Const it will become necessary to store the Ty<'tcx> inside of Expr<'tcx>. In order to do this without blowing up the size of ConstKind, we start storing the type/const args as GenericArgs

r? @oli-obk

@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. labels Jun 4, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 4, 2024

changes to the core type system

cc @compiler-errors, @lcnr

@BoxyUwU BoxyUwU mentioned this pull request Jun 4, 2024
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Jun 4, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 4, 2024

📌 Commit 67a73f2 has been approved by oli-obk

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 Jun 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
@bors
Auto merge of rust-lang#125958 - BoxyUwU:remove_const_ty, r=<try>
b8e2677 
Remove the `ty` field from type system `Const`s

Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44

Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes)

This is based on top of rust-lang#125967 and rust-lang#125968, and as such should not be merged until those are merged.

---

Why do this?
- The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs.
- As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>`
- It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`.
- Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself.
- A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅

See rust-lang#125671's description for some more information about the `ty` field

---

General summary of changes in this PR:

- Change how we represent `ty::Expr` from `generic_const_exprs` to not explode the size of `Const` in this PR (This doesn't really matter, I just didn't want to Completely break generic const exprs just yet)
- Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise.
-  Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`.
- Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`.
- SMIR represents mir consts and ty consts using the same `Const` type. This... does not work at all anymore so I have split them into two different types in SMIR and it seems to make everything a lot nicer regardless of this PR.
- rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this.

---

r? `@lcnr` `@compiler-errors`
@compiler-errors compiler-errors mentioned this pull request Jun 4, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
@bors
Auto merge of rust-lang#125976 - compiler-errors:rollup-xt3le7w, r=co…
30ea1a2 
…mpiler-errors

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125667 (Silence follow-up errors directly based on error types and regions)
 - rust-lang#125717 (Refactor `#[diagnostic::do_not_recommend]` support)
 - rust-lang#125795 (Improve renaming suggestion for names with leading underscores)
 - rust-lang#125865 (Fix ICE caused by ignoring EffectVars in type inference)
 - rust-lang#125953 (Streamline `nested` calls.)
 - rust-lang#125959 (Reduce `pub` exposure in `rustc_mir_build`)
 - rust-lang#125967 (Split smir `Const` into `TyConst` and `MirConst`)
 - rust-lang#125968 (Store the types of `ty::Expr` arguments in the `ty::Expr`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a5dc684 into rust-lang:master Jun 4, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
@rust-timer
Unrolled build for rust-lang#125968
6f46db0 
Rollup merge of rust-lang#125968 - BoxyUwU:shrink_ty_expr, r=oli-obk

Store the types of `ty::Expr` arguments in the `ty::Expr`

Part of rust-lang#125958

In attempting to remove the `ty` field on `Const` it will become necessary to store the `Ty<'tcx>` inside of `Expr<'tcx>`. In order to do this without blowing up the size of `ConstKind`, we start storing the type/const args as `GenericArgs`

r? `@oli-obk`
@rustbot rustbot added this to the 1.80.0 milestone Jun 4, 2024
@compiler-errors
Copy link
Member

@rust-timer build 6f46db0

cc #125976 (comment)

@rust-timer

This comment has been minimized.

Sign in to view
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
@bors
Auto merge of rust-lang#125958 - BoxyUwU:remove_const_ty, r=lcnr,aDot…
758ff39 
…InTheVoid

Remove the `ty` field from type system `Const`s

Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44

Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes)

This is based on top of rust-lang#125967 and rust-lang#125968, and as such should not be merged until those are merged.

---

Why do this?
- The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs.
- As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>`
- It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`.
- Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself.
- A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅

See rust-lang#125671's description for some more information about the `ty` field

---

General summary of changes in this PR:

- Change how we represent `ty::Expr` from `generic_const_exprs` to not explode the size of `Const` in this PR (This doesn't really matter, I just didn't want to Completely break generic const exprs just yet)
- Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise.
-  Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`.
- Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`.
- SMIR represents mir consts and ty consts using the same `Const` type. This... does not work at all anymore so I have split them into two different types in SMIR and it seems to make everything a lot nicer regardless of this PR.
- rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this.

---

r? `@lcnr` `@compiler-errors`
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6f46db0): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

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.7% [0.2%, 1.3%] 21
Regressions ❌
(secondary)		 0.5% [0.5%, 0.5%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.7% [0.2%, 1.3%] 21

Max RSS (memory usage)

Results (primary 2.2%)

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

Cycles

Results (primary -3.7%, secondary 6.2%)

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

Binary size

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

Bootstrap: 669.797s -> 668.959s (-0.13%)
Artifact size: 318.88 MiB -> 318.80 MiB (-0.02%)

@rustbot rustbot added the perf-regression Performance regression. label Jun 5, 2024
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jun 5, 2024 •
edited
Loading

This.... made Const smaller and other than that only affects generic_const_exprs. Absolutely zero reason why this would cause a perf regression?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 5, 2024

Just a cycles regression, no wall time changes. Likely just code shifted around by the optimizer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
perf-regression Performance regression. 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.
Projects
None yet
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.
7 participants
@BoxyUwU @rustbot @rust-log-analyzer @oli-obk @bors @compiler-errors @rust-timer