-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Cranelift: shrink ABIArgSlot #8163
base: main
Are you sure you want to change the base?
Conversation
ABIArgSlot is currently 16 bytes with 4 bytes of padding. Shrinking the offset field in its Stack variant from i64 to i32 therefore reduces the enum to 8 bytes. It also reduces its alignment to 4 bytes, which might reduce padding in containing structures. Some targets already limit stack frame sizes to 2GB or less, and in practice they must be much smaller than that, so an i32 is plenty. By making this type only one word in size, we can put two of them in ABIArgSlotVec for free, which guarantees that such SmallVecs will never spill to the heap. In principle we could use arrayvec or something that doesn't support spilling to the heap at all. There are many other places where we use i64 for stack frame offsets which would probably benefit from switching to i32, but I didn't want to change everything at once.
@@ -320,7 +320,7 @@ impl ABIMachineSpec for AArch64MachineDeps { | |||
Some((ty, slot_offset)) | |||
}) | |||
.map(|(ty, offset)| ABIArgSlot::Stack { | |||
offset, | |||
offset: i32::try_from(offset).unwrap(), |
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.
We don't currently check that the stack frame size is less than 2GB, so this unwrap can panic: #7916
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.
That's good to know! I'll hold off on merging this until I have a chance to think more about that or somebody else fixes it.
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.
LGTM, assuming we unblock it.
ABIArgSlot is currently 16 bytes with 4 bytes of padding. Shrinking the offset field in its Stack variant from i64 to i32 therefore reduces the enum to 8 bytes. It also reduces its alignment to 4 bytes, which might reduce padding in containing structures.
Some targets already limit stack frame sizes to 2GB or less, and in practice they must be much smaller than that, so an i32 is plenty.
By making this type only one word in size, we can put two of them in ABIArgSlotVec for free, which guarantees that such SmallVecs will never spill to the heap. In principle we could use arrayvec or something that doesn't support spilling to the heap at all.
There are many other places where we use i64 for stack frame offsets which would probably benefit from switching to i32, but I didn't want to change everything at once.