-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
Change SwitchTargets to use ScalarInt #161033
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1732,11 +1732,11 @@ mod size_asserts { | |
|
|
||
| use super::*; | ||
| // tidy-alphabetical-start | ||
| static_assert_size!(BasicBlockData<'_>, 144); | ||
| static_assert_size!(BasicBlockData<'_>, 152); | ||
| static_assert_size!(LocalDecl<'_>, 40); | ||
| static_assert_size!(SourceScopeData<'_>, 64); | ||
| static_assert_size!(Statement<'_>, 40); | ||
| static_assert_size!(Terminator<'_>, 104); | ||
| static_assert_size!(Terminator<'_>, 112); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Terminator size changes had caused regressions before, so this is quite unfortunate, but it's also somewhat mysterious, because the effect is not always there. I tried to reduce SwitchInt size by using slightly different representation in #159928, but it didn't have as much effect (but I also had to change other Terminator variants to make it smaller, and increase its size in metadata, so those changes could erase the wins from There's definitely a way to make |
||
| static_assert_size!(VarDebugInfo<'_>, 88); | ||
| // tidy-alphabetical-end | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Isn't the new code here checking if
targetscontains a u128 here? I would think this code would cause this check to not apply whentargetsis of typeu8, a 1-byte ScalarInt. But surely that's wrong... right?View changes since the review
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.
I think it's fine since the
add_target()takes au128, and thevaluesinSwitchTargetsispub(super), so au8probably wouldn't end up here?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.
Oh. So even though this change does technically use
ScalarIntin the targets, all thoseScalarIntare secretlyu128, which means that the one-byte overhead is used for a constant16. I think this change is actually incomplete then, and the API ofSwitchTargetswould need to be in terms ofScalarInt, notu128. That is a more invasive change, but based on the way @RalfJung wrote the linked issue, should result in some cleanup for users of the API.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.
I see. I just took a rough look at how users are using these APIs and found several cases using
as u128or evenScalarInt::to_bits(). I'd take some time to do the cleanup thenThere 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.
Yeah the entire point of that issue was that the ScalarInt stored here should actually have a size that matches the operand we are working on. :) Then e.g. the interpreter can work with that ScalarInt directly rather than having to go via
u128.