neon: drop the align requirement on vld1/vst1/vld1 lane loads and stores, dup loads - #2214
Open
valentynkit wants to merge 3 commits into
Open
neon: drop the align requirement on vld1/vst1/vld1 lane loads and stores, dup loads#2214valentynkit wants to merge 3 commits into
valentynkit wants to merge 3 commits into
Conversation
vld1 and vst1 lane intrinsics were dereferencing pointer directly which requires it to be aligned to the element type. (Too strict alignment requirements) The instructions don't requires alignment, so calling these with unaligned pointer caused UB. Lane loads and stores were updated to use `read_unaligned()` and `write_unaligned()` instead.
Collaborator
|
Thanks for the pull request, and welcome! The Rust Project has assigned @sayantn (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks. Why was this reviewer chosen?The reviewer was selected based on:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
vld1/vst1 lane functions and vld1 dup functions were de-referencing pointer directly which requires it to be aligned to the element type. (Too strict alignment requirements)
The instructions don't requires alignment, so calling these with unaligned pointer caused UB.
Lane loads and stores, and dup loads were updated to use
read_unaligned()andwrite_unaligned()instead.Use crate::ptr::read_unaligned(ptr) instead of ptr.read_unaligned() because the same convention is used in neighboring code. Let me know if the other approach seems cleaner and should be used instead.
Also on armv7 the 64-bit loads now compile to
vld1.8instead ofvldr(vldrrequires an aligned address), so their armassert_instrvalues updated to match. Without this the tests would fail after getting rid of alignment requirements in the function, and the f32 lane and dup loads LLVM emitsldr+vmov/vdup.32on arm instead of a NEON load form, so those four asserts now expectldr. Is this acceptable, or I am missing something here, it feels pretty unreliable to change assertions, but not sure if there are a better way?Align requirement documented in:
ARMv7-A and ARMv7-R Manual ch A3.2.1
This covers a bit more than the reported issue required to fix it, but there look to be more places with the same category of bug, because implementation requiring stricter alignment that instructions do.
I could take a look at those as follow-ups, if it will be valuable.
Fixes #2198