seanmonstar / httparse

A push parser for the HTTP 1.x protocol in Rust.
https://docs.rs/httparse
Apache License 2.0
567 stars 111 forks source link

fix: neon no_std #141

Closed AaronO closed 1 year ago

AaronO commented 1 year ago

I accidentally typed std::mem out of habit, this wasn't caught in #133 because neon+no_std isn't exercised

seanmonstar commented 1 year ago

I was digging through the docs (I wanted to check on the transmute), and noticed it wasn't stabilized until 1.59? Woops, oh well :)

seanmonstar commented 1 year ago

So, it looks it's an uint64x1_t, which doesn't have a repr(transparent) on it. Is there it publicly documented somewhere that we can transmute it into the primitive? That's not usually allowed (transmuting a unit struct to the field isn't guaranteed to be right).

AaronO commented 1 year ago

So, it looks it's an uint64x1_t, which doesn't have a repr(transparent) on it. Is there it publicly documented somewhere that we can transmute it into the primitive? That's not usually allowed (transmuting a unit struct to the field isn't guaranteed to be right).

The docs describe it as:

ARM-specific 64-bit wide vector of one packed u64.

The code is:

/// ARM-specific 64-bit wide vector of one packed `u64`.
#[cfg_attr(not(target_arch = "arm"), stable(feature = "neon_intrinsics", since = "1.59.0"))]
pub struct uint64x1_t(pub(crate) u64);

It's obviously correct. They transmute in their own tests to u64 => uint64x1_t, but I guess the idiomatic way would be vget_lane_u64 should compile to the same code moving the values from the SIMD registers to general purpose registers.

AaronO commented 1 year ago

I was digging through the docs (I wanted to check on the transmute), and noticed it wasn't stabilized until 1.59? Woops, oh well :)

Yeah, I guess we could compile-time check for neon_intrinsics or the rust version guard on that and fallback to SWAR ? (Rust 1.59 is 1yo, still not bad but understand why you want to be conservative)

seanmonstar commented 1 year ago

Yea, I know it works transmuting it now, but the transmute docs point out it's not "defined", so it can't be depended on:

For repr(C) types and repr(transparent) types, layout is precisely defined. But for your run-of-the-mill repr(Rust), it is not.

repr(simd) isn't really documented there at all. This also includes structs with a single field, they technically could have a different layout than the field alone, even if they currently don't. 🤷

(Rust 1.59 is 1yo, still not bad but understand why you want to be conservative)

The biggest "old" one would be Debian, which is really close to that range. Some other environments have even older compilers.

AaronO commented 1 year ago

@seanmonstar See #142, added a test for aarch64 on MSRV and guarded it so we fallback to swar if < 1.59