Wasmtime GitHub notifications bot (May 16 2024 at 15:21): Wasmtime GitHub notifications bot (May 16 2024 at 15:21):cfallin requested fitzgen for a review on PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 15:21):cfallin requested wasmtime-compiler-reviewers for a review on PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 15:21):cfallin requested wasmtime-core-reviewers for a review on PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 15:21):cfallin opened PR #8635 from cfallin:stackslot-alignment to bytecodealliance:main:
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
StackSlotDatato 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.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (May 16 2024 at 15:23):cfallin submitted PR review.
Wasmtime GitHub notifications bot (May 16 2024 at 15:23):cfallin created PR review comment:
(Just below this point) the old
alignmentfunction didn't seem to be used in the ABI code or anywhere else; it's likely left over from some earlier use-case. The "largest power of two that divides..." logic is a little weird (it can result in an alignment less than requested?) so I removed it and pushed the alignment computation to thestd::cmp::maxinabi.rs.
Wasmtime GitHub notifications bot (May 16 2024 at 15:27):cfallin updated PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 15:28):cfallin commented on PR #8635:
(for the record: work on this now spawned by this thread)
Wasmtime GitHub notifications bot (May 16 2024 at 15:48):elliottt submitted PR review:
Awesome!
Wasmtime GitHub notifications bot (May 16 2024 at 15:48):elliottt submitted PR review:
Awesome!
Wasmtime GitHub notifications bot (May 16 2024 at 15:48):elliottt created PR review comment:
Do we have a way of enabling an explicit stack check in clif tests? That would be a nice way of verifying that the total stack space required is accounted for.
Wasmtime GitHub notifications bot (May 16 2024 at 15:48):elliottt created PR review comment:
u32::try_from(align).map_err(|_| self.error("alignment must be a 32-bit unsigned integer"))?
Wasmtime GitHub notifications bot (May 16 2024 at 15:55):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 16 2024 at 15:55):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 16 2024 at 15:55):alexcrichton created PR review comment:
Can this be set to 4? The ValRaw bits in Wasmtime require a 16-byte alignment
Wasmtime GitHub notifications bot (May 16 2024 at 16:05):elliottt submitted PR review.
Wasmtime GitHub notifications bot (May 16 2024 at 16:05):elliottt created PR review comment:
Ah!
function %f0() -> i64, i64, i64, i64 { gv0 = vmctx ss0 = explicit_slot 8, align=16 ss1 = explicit_slot 8, align=16 ss2 = explicit_slot 4 ss3 = explicit_slot 4 ss4 = stack_limit gv0
Wasmtime GitHub notifications bot (May 16 2024 at 16:06):elliottt edited PR review comment.
Wasmtime GitHub notifications bot (May 16 2024 at 16:18):cfallin updated PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 16:20):cfallin updated PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 16:21):cfallin updated PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 16:21):cfallin submitted PR review.
Wasmtime GitHub notifications bot (May 16 2024 at 16:21):cfallin created PR review comment:
Done!
Wasmtime GitHub notifications bot (May 16 2024 at 16:24):cfallin updated PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 16:25):cfallin has enabled auto merge for PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 16:26):cfallin updated PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 17:02):cfallin merged PR #8635.
Wasmtime GitHub notifications bot (May 16 2024 at 19:50):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (May 16 2024 at 19:50):bjorn3 created PR review comment:
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.
Wasmtime GitHub notifications bot (May 16 2024 at 21:13):cfallin submitted PR review.
Wasmtime GitHub notifications bot (May 16 2024 at 21:13):cfallin created PR review comment:
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.
Last updated: Oct 23 2024 at 20:03 UTC