Skip to content
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: add alignment parameter to stack slots. #8635

Merged
merged 6 commits into from
May 16, 2024

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented May 16, 2024

Fixes #6716.

Currently, stack slots on the stack are aligned only to a machine-word boundary. This is insufficient for some use-cases: for example, storing SIMD data or structs that require a larger alignment.

This PR adds a parameter to the StackSlotData to specify alignment, and the associated logic to the CLIF parser and printer. It updates the shared ABI code to compute the stackslot layout taking the alignment into account. In order to ensure the alignment is always a power of two, it is stored as a shift amount (log2 of actual alignment) in the IR.

@cfallin cfallin requested review from a team as code owners May 16, 2024 15:21
@cfallin cfallin requested review from fitzgen and removed request for a team May 16, 2024 15:21
Fixes bytecodealliance#6716.

Currently, stack slots on the stack are aligned only to a machine-word
boundary. This is insufficient for some use-cases: for example, storing
SIMD data or structs that require a larger alignment.

This PR adds a parameter to the `StackSlotData` to specify alignment,
and the associated logic to the CLIF parser and printer. It updates the
shared ABI code to compute the stackslot layout taking the alignment
into account. In order to ensure the alignment is always a power of two,
it is stored as a shift amount (log2 of actual alignment) in the IR.
@cfallin cfallin force-pushed the stackslot-alignment branch from 0855496 to 6d96151 Compare May 16, 2024 15:27
@cfallin
Copy link
Member Author

cfallin commented May 16, 2024

(for the record: work on this now spawned by this thread)

Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Awesome!

cranelift/filetests/filetests/isa/x64/stackslot.clif Outdated Show resolved Hide resolved
cranelift/reader/src/parser.rs Outdated Show resolved Hide resolved
@cfallin cfallin enabled auto-merge May 16, 2024 16:25
@cfallin cfallin added this pull request to the merge queue May 16, 2024
Merged via the queue into bytecodealliance:main with commit 45bc736 May 16, 2024
28 checks passed
@cfallin cfallin deleted the stackslot-alignment branch May 16, 2024 17:02
// satisfy the user's requested alignment.
debug_assert!(data.align_shift < 32);
let align = std::cmp::max(M::word_bytes(), 1u32 << data.align_shift);
let mask = align - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the alignment is bigger than the ABI stack alignment it would be necessary to adjust the stack pointer to correctly align it. This is missing here. Rust allows arbitrary power of two alignments for stack variables, even something crazy like a 1GB alignment. More realistically though, cache line alignment is used by crates like rayon for performance reasons. Without correctly aligning, rustc will trigger a debug assertion in some cases like for the aforementioned rayon crate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, that's a good point -- to summarize: alignment of offset within the stack frame to alignment A does not imply alignment to A overall, because SP itself may be aligned less (e.g. only to A/4).

I don't have time to work further on this at the moment, but I'd be happy to review a PR that addressed this.

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.

Support specifying stack slot alignments
4 participants