-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
MGCA: Support struct expressions without intermediary anon consts #149114
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
Conversation
|
|
0dbf003 to
a019ac7
Compare
This comment has been minimized.
This comment has been minimized.
a019ac7 to
b557322
Compare
This comment has been minimized.
This comment has been minimized.
b557322 to
734737a
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
MGCA: Support struct expressions without intermediary anon consts
This comment has been minimized.
This comment has been minimized.
| let (variant_idx, branches) = if def.is_enum() { | ||
| let (head, rest) = branches.split_first().unwrap(); | ||
| (VariantIdx::from_u32(head.unwrap_leaf().to_u32()), rest) | ||
| (VariantIdx::from_u32(head.to_value().valtree.unwrap_leaf().to_u32()), rest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the addition of all the .to_value().valtree everywhere is kind of unfortunate
| // We need a branch for each "level" of the data structure. | ||
| let bytes = ty::ValTree::from_raw_bytes(tcx, byte_sym.as_byte_str()); | ||
| ty::ValTree::from_branches(tcx, [bytes]) | ||
| ty::ValTree::from_branches(tcx, [ty::Const::new_value(tcx, bytes, *inner_ty)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with all the extra calls to ty::Const::new_x
This comment has been minimized.
This comment has been minimized.
734737a to
6496c15
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (eeb4682): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.2%, secondary 1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 474.035s -> 472.627s (-0.30%) |
6496c15 to
1a44434
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (1dcb7af): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.9%, secondary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -4.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 482.897s -> 486.06s (0.66%) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final nits, then r=me
e7bba6d to
04bd0db
Compare
04bd0db to
79fd535
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r=lcnr rollup=never |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 2ca7bcd (parent) -> 8796b3b (this PR) Test differencesShow 98 test diffsStage 1
Stage 2
Additionally, 68 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 8796b3b8b4ac6f38a80bf80ce89dd7bd7f92edd7 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (8796b3b): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (primary 2.5%, secondary -2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 481.719s -> 484.054s (0.48%) |
MGCA: Support struct expressions without intermediary anon consts r? oli-obk tracking issue: rust-lang/rust#132980 Fixes rust-lang/rust#127972 Fixes rust-lang/rust#137888 Fixes rust-lang/rust#140275 due to delaying a bug instead of ICEing in HIR ty lowering. ### High level goal Under `feature(min_generic_const_args)` this PR adds another kind of const argument. A struct/variant construction const arg kind. We represent the values of the fields as themselves being const arguments which allows for uses of generic parameters subject to the existing restrictions present in `min_generic_const_args`: ```rust fn foo<const N: Option<u32>>() {} trait Trait { #[type_const] const ASSOC: usize; } fn bar<T: Trait, const N: u32>() { // the initializer of `_0` is a `N` which is a legal const argument // so this is ok. foo::<{ Some::<u32> { 0: N } }>(); // this is allowed as mgca supports uses of assoc consts in the // type system. ie `<T as Trait>::ASSOC` is a legal const argument foo::<{ Some::<u32> { 0: <T as Trait>::ASSOC } }>(); // this on the other hand is not allowed as `N + 1` is not a legal // const argument foo::<{ Some::<u32> { 0: N + 1 } }>(); } ``` This PR does not support uses of const ctors, e.g. `None`. And also does not support tuple constructors, e.g. `Some(N)`. I believe that it would not be difficult to add support for such functionality after this PR lands so have left it out deliberately. We currently require that all generic parameters on the type being constructed be explicitly specified. I haven't really looked into why that is but it doesn't seem desirable to me as it should be legal to write `Some { ... }` in a const argument inside of a body and have that desugar to `Some::<_> { ... }`. Regardless this can definitely be a follow-up PR and I assume this is some underlying consistency with the way that elided args are handled with type paths elsewhere. This PRs implementation of supporting struct expressions is somewhat incomplete. We don't handle `Foo { ..expr }` at all and aren't handling privacy/stability. The printing of `ConstArgKind::Struct` HIR nodes doesn't really exist either :') I've tried to keep the implementation here somewhat deliberately incomplete as I think a number of these issues are actually quite small and self contained after this PR lands and I'm hoping it could be a good set of issues to mentor newer contributors on 🤔 I just wanted the "bare minimum" required to actually demonstrate that the previous changes are "necessary". ### `ValTree` now recurse through `ty::Const` In order to actually represent struct/variant construction in `ty::Const` without going through an anon const we would need to introduce some new `ConstKind` variant. Let's say some hypothetical `ConstKind::ADT(Ty<'tcx>, List<Const<'tcx>>)`. This variant would represent things the same way that `ValTree` does with the first element representing the `VariantIdx` of the enum (if its an enum), and then followed by a list of field values in definition order. This *could* work but there are a few reasons why it's suboptimal. First it would mean we have a second kind of `Const` that can be normalized. Right now we only have `ConstKind::Unevaluated` which possibly needs normalization. Similarly with `TyKind` we *only* have `TyKind::Alias`. If we introduced `ConstKind::ADT` it would need to be normalized to a `ConstKind::Value` eventually. This feels to me like it has the potential to cause bugs in the long run where only `ConstKind::Unevaluated` is handled by some code paths. Secondly it would make type equality/inference be kind of... weird... It's desirable for `Some { 0: ?x } eq Some { 0: 1_u32 }` to result in `?x=1_u32`. I can't see a way for this to work with this `ConstKind::ADT` design under the current architecture for how we represent types/consts and generally do equality operations. We would need to wholly special case these two variants in type equality and have a custom recursive walker separate from the existing architecture for doing type equality. It would also be somewhat unique in that it's a non-rigid `ty::Const` (it can be normalized more later on in type inference) while also having somewhat "structural" equality behaviour. Lastly, it's worth noting that its not *actually* `ConstKind::ADT` that we want. It's desirable to extend this setup to also support tuples and arrays, or even references if we wind up supporting those in const generics. Therefore this isn't really `ConstKind::ADT` but a more general `ConstKind::ShallowValue` or something to that effect. It represents at least one "layer" of a types value :') Instead of doing this implementation choice we instead change `ValTree::Branch`: ```rust enum ValTree<'tcx> { Leaf(ScalarInt), // Before this PR: Branch(Box<[ValTree<'tcx>]>), // After this PR Branch(Box<[Const<'tcx>]>), } ``` The representation for so called "shallow values" is now the same as the representation for the *entire* full value. The desired inference/type equality behaviour just falls right out of this. We also don't wind up with these shallow values actually being non-rigid. And `ValTree` *already* supports references/tuples/arrays so we can handle those just fine. I think in the future it might be worth considering inlining `ValTree` into `ty::ConstKind`. E.g: ```rust enum ConstKind { Scalar(Ty<'tcx>, ScalarInt), ShallowValue(Ty<'tcx>, List<Const<'tcx>>), Unevaluated(UnevaluatedConst<'tcx>), ... } ``` This would imply that the usage of `ValTree`s in patterns would now be using `ty::Const` but they already kind of are anyway and I think that's probably okay in the long run. It also would mean that the set of things we *could* represent in const patterns is greater which may be desirable in the long run for supporting things such as const patterns of const generic parameters. Regardless, this PR doesn't actually inline `ValTree` into `ty::ConstKind`, it only changes `Branch` to recurse through `Const`. This change could be split out of this PR if desired. I'm not sure if there'll be a perf impact from this change. It's somewhat plausible as now all const pattern values that have nesting will be interning a lot more `Ty`s. We shall see :> ### Forbidding generic parameters under mgca Under mgca we now allow all const arguments to resolve paths to generic parameters. We then *later* actually validate that the const arg should be allowed to access generic parameters if it did wind up resolving to any. This winds up just being a lot simpler to implement than trying to make name resolution "keep track" of whether we're inside of a non-anon-const const arg and then encounter a `const { ... }` indicating we should now stop allowing resolving to generic parameters. It's also somewhat in line with what we'll need for a `feature(generic_const_args)` where we'll want to decide whether an anon const should have any generic parameters based off syntactically whether any generic parameters were used. Though that design is entirely hypothetical at this point :) ### Followup Work - Make HIR ty lowering check whether lowering generic parameters is supported and if not lower to an error type/const. Should make the code cleaner, fix some other bugs, and maybe(?) recover perf since we'll be accessing less queries which I think is part of the perf regression of this PR - Make the ValTree setup less scuffed. We should find a new name for `ConstKind::Value` and the `Val` part of `ValTree` and `ty::Value` as they no longer correspond to a fully normalized structure. It may also be worth looking into inlining `ValTreeKind` into `ConstKind` or atleast into `ty::Value` or sth 🤔 - Support tuple constructors and const constructors not just struct expressions. - Reduce code duplication between HIR ty lowering's handling of struct expressions, and HIR typeck's handling of struct expressions - Try fix perf rust-lang/rust#149114 (comment). Maybe this will clear up once we clean up `ValTree` a bit and stop doing double interning and whatnot
r? oli-obk
tracking issue: #132980
Fixes #127972
Fixes #137888
Fixes #140275
due to delaying a bug instead of ICEing in HIR ty lowering.
High level goal
Under
feature(min_generic_const_args)this PR adds another kind of const argument. A struct/variant construction const arg kind. We represent the values of the fields as themselves being const arguments which allows for uses of generic parameters subject to the existing restrictions present inmin_generic_const_args:This PR does not support uses of const ctors, e.g.
None. And also does not support tuple constructors, e.g.Some(N). I believe that it would not be difficult to add support for such functionality after this PR lands so have left it out deliberately.We currently require that all generic parameters on the type being constructed be explicitly specified. I haven't really looked into why that is but it doesn't seem desirable to me as it should be legal to write
Some { ... }in a const argument inside of a body and have that desugar toSome::<_> { ... }. Regardless this can definitely be a follow-up PR and I assume this is some underlying consistency with the way that elided args are handled with type paths elsewhere.This PRs implementation of supporting struct expressions is somewhat incomplete. We don't handle
Foo { ..expr }at all and aren't handling privacy/stability. The printing ofConstArgKind::StructHIR nodes doesn't really exist either :')I've tried to keep the implementation here somewhat deliberately incomplete as I think a number of these issues are actually quite small and self contained after this PR lands and I'm hoping it could be a good set of issues to mentor newer contributors on 🤔 I just wanted the "bare minimum" required to actually demonstrate that the previous changes are "necessary".
ValTreenow recurse throughty::ConstIn order to actually represent struct/variant construction in
ty::Constwithout going through an anon const we would need to introduce some newConstKindvariant. Let's say some hypotheticalConstKind::ADT(Ty<'tcx>, List<Const<'tcx>>).This variant would represent things the same way that
ValTreedoes with the first element representing theVariantIdxof the enum (if its an enum), and then followed by a list of field values in definition order.This could work but there are a few reasons why it's suboptimal.
First it would mean we have a second kind of
Constthat can be normalized. Right now we only haveConstKind::Unevaluatedwhich possibly needs normalization. Similarly withTyKindwe only haveTyKind::Alias. If we introducedConstKind::ADTit would need to be normalized to aConstKind::Valueeventually. This feels to me like it has the potential to cause bugs in the long run where onlyConstKind::Unevaluatedis handled by some code paths.Secondly it would make type equality/inference be kind of... weird... It's desirable for
Some { 0: ?x } eq Some { 0: 1_u32 }to result in?x=1_u32. I can't see a way for this to work with thisConstKind::ADTdesign under the current architecture for how we represent types/consts and generally do equality operations.We would need to wholly special case these two variants in type equality and have a custom recursive walker separate from the existing architecture for doing type equality. It would also be somewhat unique in that it's a non-rigid
ty::Const(it can be normalized more later on in type inference) while also having somewhat "structural" equality behaviour.Lastly, it's worth noting that its not actually
ConstKind::ADTthat we want. It's desirable to extend this setup to also support tuples and arrays, or even references if we wind up supporting those in const generics. Therefore this isn't reallyConstKind::ADTbut a more generalConstKind::ShallowValueor something to that effect. It represents at least one "layer" of a types value :')Instead of doing this implementation choice we instead change
ValTree::Branch:The representation for so called "shallow values" is now the same as the representation for the entire full value. The desired inference/type equality behaviour just falls right out of this. We also don't wind up with these shallow values actually being non-rigid. And
ValTreealready supports references/tuples/arrays so we can handle those just fine.I think in the future it might be worth considering inlining
ValTreeintoty::ConstKind. E.g:This would imply that the usage of
ValTrees in patterns would now be usingty::Constbut they already kind of are anyway and I think that's probably okay in the long run. It also would mean that the set of things we could represent in const patterns is greater which may be desirable in the long run for supporting things such as const patterns of const generic parameters.Regardless, this PR doesn't actually inline
ValTreeintoty::ConstKind, it only changesBranchto recurse throughConst. This change could be split out of this PR if desired.I'm not sure if there'll be a perf impact from this change. It's somewhat plausible as now all const pattern values that have nesting will be interning a lot more
Tys. We shall see :>Forbidding generic parameters under mgca
Under mgca we now allow all const arguments to resolve paths to generic parameters. We then later actually validate that the const arg should be allowed to access generic parameters if it did wind up resolving to any.
This winds up just being a lot simpler to implement than trying to make name resolution "keep track" of whether we're inside of a non-anon-const const arg and then encounter a
const { ... }indicating we should now stop allowing resolving to generic parameters.It's also somewhat in line with what we'll need for a
feature(generic_const_args)where we'll want to decide whether an anon const should have any generic parameters based off syntactically whether any generic parameters were used. Though that design is entirely hypothetical at this point :)Followup Work
ConstKind::Valueand theValpart ofValTreeandty::Valueas they no longer correspond to a fully normalized structure. It may also be worth looking into inliningValTreeKindintoConstKindor atleast intoty::Valueor sth 🤔ValTreea bit and stop doing double interning and whatnot