docs(linalg): fill in the empty Safety section on load_unaligned - #8885
docs(linalg): fill in the empty Safety section on load_unaligned#8885LuciferYang wants to merge 1 commit into
Conversation
b5de0d0 to
4caed81
Compare
4caed81 to
f65685f
Compare
|
Taking the first of your two options rather than the ordering: the contract now requires the target features generically, so it is complete for LASX, for x86 AVX, and for any arm added later, and it does not go stale when #8874 lands. Not gating this on #8874 merging first. #8874 fixes three of the four x86 register types, |
f65685f to
7dc671d
Compare
There was a problem hiding this comment.
This head accurately documents the current pointer, AVX, LASX, and baseline-safe u8x16 obligations. The explicit x86 matrix is coupled to #8874, which replaces those AVX loads but does not yet update this text; that PR should revise the x86 requirement when it rebases or merges. No change is required here before this revision can land.
What this changes
SIMD::load_unalignedhad a# Safetyheading with nothing under it:Its two neighbours in the same trait,
loadandstore, both say what they require. The trait ispub, so this renders as an empty section, andclippy::missing_safety_docis satisfied by the heading alone and says nothing.The section now states the pointer requirement plus the one a caller cannot guess: five of the six implementations reach an intrinsic that needs a target feature the build's baseline may not provide, and none of them check at runtime. On x86_64 that is
_mm256_loadu_*, which carries#[target_feature(enable = "avx")]incore::arch, so calling it without AVX is undefined behaviour rather than a wrong answer. That is the gap in #8872.u8x16is called out separately because it is the exception:_mm_loadu_si128is SSE2,vld1q_u8is baseline NEON, and everywhere else it is a scalar loop, so it needs nothing extra on any target. Lumping it in with the others would push callers of the one safe implementation into detection they do not need.Why the section is longer than the bug
Two shorter versions were each wrong in a different direction, and both are worth recording so the next person does not retry them.
"Unlike [
SIMD::load] it carries no alignment requirement" is false for most implementations, because for most types the two methods are the same code: on aarch64loadcallsload_unaligneddirectly forf32x8,f32x16,f64x4andf64x8, and on x86_64i32x8::loadandu8x16::loaduse the same unaligned intrinsic. The alignment distinction exists only in the four x86_64_mm256_load_ps/_mm256_load_pdarms, so the sentence would also have ratifiedload's own doc, which claims the crash for every type."An implementation may also require target features the generic baseline does not imply" is true but not dischargeable. A caller writing
unsafe { u8x16::load_unaligned(p) }has to satisfy the stated obligation, and an existential over unnamed features on unnamed targets leaves only two readings: assume the worst, or ignore the clause. Naming the features, the granularity (the build's baseline, not the architecture:.cargo/config.tomlsetstarget-cpu=haswellforx86_64-unknown-linux-gnu, so in-repo builds do imply AVX while a downstream build of the published crate does not) and the mechanism is what makes it usable. The sibling docs in the same module already nameis_x86_feature_detected!this way.The pre-existing wording on
loadandstoreis left alone. Correcting it is a decision about what those methods should promise, not a docs fix.Test plan
Documentation only, no behavior change.
RUSTDOCFLAGS="-D warnings -D rustdoc::broken_intra_doc_links" cargo doc -p lance-linalg --no-deps: cleancargo fmt --all -- --checkandcargo clippy -p lance-linalg --all-targets -- -D warnings: cleanload_unalignedimplementation:f32.rs:214and:612,f64.rs:111and:467,i32.rs:118,u8.rs:157