-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Stop making const None in GVN
#138526
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
Stop making const None in GVN
#138526
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Having `ALLOC`s for these seems quite unhelpful, since it's extra stuff to keep track of, makes it harder for other passes to know what's in it (reversing a tag constant to a `VariantIdx` isn't trivial), and in codegen it's arguably harder to get a `None` from a valtree than from seeing "oh, aggregate nothing with this `VariantIdx`". Making constants for the `discriminant(_n)` is useful, and still happens. Constants for primitives are helpful; for enums not so much.
|
r? compiler |
This comment has been minimized.
This comment has been minimized.
… *not* marking the issue as fixed, because it's really not. (There MIR tests really need to be regressed with MIR tests, not with UI tests.)
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Stop making `const None` in GVN Having `ALLOC`s for these seems unhelpful, since it's extra stuff to keep track of, makes it harder for other passes to know what's in it (reversing a tag constant to a `VariantIdx` isn't trivial), and even in codegen it's arguably harder to get a `None` from a valtree than from seeing "oh, aggregate nothing with this `VariantIdx`". Making constants for the `discriminant(_n)` is useful for putting in `switchInt`s, and still happens with this PR. It's just for the full enum that we don't make consts (except for might-as-well-be-an-integer enums like `Ordering`, or ones that are actually ZSTs like `const Option::<Infallible>::None`). This also adds a couple tests I've been using as I look at how we're doing enums in MIR. They're not really *improved* by this change materially, but they help motivate why I'm trying to tweak how the enums come out.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e18ec78): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -0.2%, secondary 3.0%)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.
CyclesResults (primary 5.5%, secondary 2.8%)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.
Binary sizeResults (primary -0.0%, secondary -0.1%)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.
Bootstrap: 774.866s -> 772.816s (-0.26%) |
|
r? mir-opt |
|
nvm, looks like this is bad on its own and will need other things before it can be considered. |
Having
ALLOCs for these seems unhelpful, since it's extra stuff to keep track of, makes it harder for other passes to know what's in it (reversing a tag constant to aVariantIdxisn't trivial), and even in codegen it's arguably harder to get aNonefrom a valtree than from seeing "oh, aggregate nothing with thisVariantIdx".Making constants for the
discriminant(_n)is useful for putting inswitchInts, and still happens with this PR. It's just for the full enum that we don't make consts (except for might-as-well-be-an-integer enums likeOrdering, or ones that are actually ZSTs likeconst Option::<Infallible>::None).This also adds a couple tests I've been using as I look at how we're doing enums in MIR. They're not really improved by this change materially, but they help motivate why I'm trying to tweak how the enums come out.