Split smir Const into TyConst and MirConst
#125967
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 4, 2024
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`
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 4, 2024
…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
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 4, 2024
Rollup merge of rust-lang#125967 - BoxyUwU:split_smir_const, r=oli-obk Split smir `Const` into `TyConst` and `MirConst` Part of rust-lang#125958 Building a `smir::Const` currently requires accessing the `Ty<'tcx>` of a `ty::Const`. This will stop being possible in the future. Replicate the split in rustc of having a representation of type level constants and mir constants with the latter being able to store the former. Ideally we wouldnt have `MirConst::Ty` but 🤷♀️ r? `@oli-obk`
|
@rust-timer build 93bd584 |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 4, 2024
…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`
|
Finished benchmarking commit (93bd584): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking 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. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 669.797s -> 671.275s (0.22%) |
This was referenced Jun 13, 2024
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Jun 15, 2024
…-obk smir: merge identical Constant and ConstOperand types The first commit renames the const operand visitor functions on regular MIR to match the type name, that was forgotten in the original rename. The second commit changes stable MIR, fixing rust-lang/project-stable-mir#71. Previously there were two different smir types for the MIR type `ConstOperand`, one used in `Operand` and one in `VarDebugInfoContents`. Maybe we should have done this with rust-lang#125967, so there's only a single breaking change... but I saw that PR too late. Fixes rust-lang/project-stable-mir#71
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 15, 2024
Rollup merge of rust-lang#126410 - RalfJung:smir-const-operand, r=oli-obk smir: merge identical Constant and ConstOperand types The first commit renames the const operand visitor functions on regular MIR to match the type name, that was forgotten in the original rename. The second commit changes stable MIR, fixing rust-lang/project-stable-mir#71. Previously there were two different smir types for the MIR type `ConstOperand`, one used in `Operand` and one in `VarDebugInfoContents`. Maybe we should have done this with rust-lang#125967, so there's only a single breaking change... but I saw that PR too late. Fixes rust-lang/project-stable-mir#71
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #125958
Building a
smir::Constcurrently requires accessing theTy<'tcx>of aty::Const. This will stop being possible in the future. Replicate the split in rustc of having a representation of type level constants and mir constants with the latter being able to store the former. Ideally we wouldnt haveMirConst::Tybut 🤷♀️r? @oli-obk