Skip to content

Conversation

@srosefield-riverside
Copy link

Cranelift now supports explicit alignment for stack slots so we can allocate the requested type alignment when it can be guaranteed by the target.

@srosefield-riverside srosefield-riverside force-pushed the feature/reduced_stack_alignment_requirements branch from 6094403 to 95fbd7e Compare November 12, 2025 17:00
});
Pointer::stack_slot(stack_slot)
} else {
// Alignment is too big to handle using the above hack. Dynamically realign a stack slot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adjust this comment. IIUC, you're getting rid of the "hack".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dynamic realignment is still necessary when align > abi_align due to Cranelift limitations.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant "the hack above" no longer refers to the old code's size/abi_align*abi_align.

Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual code changes LGTM. Please adjust the assertion to a debug assertion and adjust the comment as requested by @swasey.

size.is_multiple_of(align),
"size must be a multiple of alignment (size={size}, align={align})"
);
assert!(align.is_power_of_two(), "alignment must be a power of two (align={align})");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a debug assert. Unlike the above assertion, I don't see it very likely that it will fail even while actively making changes to cg_clif.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants