-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Represent MIR composite debuginfo as projections instead of aggregates #115252
Conversation
r? @davidtwco (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
eda81d5
to
1cac0ae
Compare
@bors r+ |
📌 Commit 1cac0ae5e259e288045675553ff5d54526892d30 has been approved by It is now in the queue for this repository. |
⌛ Testing commit 1cac0ae5e259e288045675553ff5d54526892d30 with merge 94aacbc28dfbcef5b8cd22ed74718ea99015cfb8... |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
No idea what failed. |
⌛ Testing commit 1cac0ae5e259e288045675553ff5d54526892d30 with merge 94c65b155804f7dbc7889935b6dea74e1318b08a... |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
1cac0ae
to
424883b
Compare
424883b
to
26c48e6
Compare
Are you waiting on another review? |
I changed the implementation a little to simplify formatting. See the first commit. Could you take a second look? |
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (a5b2ac6): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression 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)ResultsThis 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.
CyclesResultsThis 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 sizeResultsThis 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: 627.776s -> 629.849s (0.33%) |
@cjgillot: this change regressed binary size by a small amount across many benchmarks. Was that expected? Could it be avoided? |
From the stats, I understand that the regression is mostly in metadata size. That can be explained by the data structure change. The representation is more general, but unfortunately less compact: it repeats the type several times. I'm not sure how to shrink it back though. |
Composite debuginfo for MIR is currently represented as
ie. a single
VarDebugInfo
object with that name, and its value aVarDebugInfoContents::Composite
.This PR proposes to reverse the representation to be
ie. multiple
VarDebugInfo
objects with each their projection.This simplifies the handling of composite debuginfo by the compiler by avoiding weird nesting.
Based on #115139