Add portable HighwayHash to hwy/contrib/hash (#3246) - #3334
Conversation
Re-implements HighwayHash (arxiv.org/abs/1612.06257) using Highway ops, so one source produces the frozen 64/128/256-bit golden values on every target instead of the reference repo's per-ISA hand-written versions. The 256-bit state (v0, v1, mul0, mul1) is mixed in 128-bit halves, matching the reference SSE4.1 code, so only Vec128 is needed and it runs on 128-bit-only targets too. Ops used: RotateRight<32>, MulEven, TableLookupBytes (ZipperMerge), RotateLeftSame (length padding) and ShiftLeftBytes/ShiftRight (the x^128+x^2+x modular reduction for the 256-bit result). Little-endian message layout; big-endian swaps lane bytes on load. Not yet specialized for wider vectors; a 256-bit-native path is a follow-up. Test hashes every length in [0, 64] against the frozen golden tables from google/highwayhash for all three output sizes, plus multi-packet inputs and a one-bit-change check. Passes on AVX2/SSE4/SSSE3/SSE2/EMU128.
jan-wassenberg
left a comment
There was a problem hiding this comment.
Nice work, thanks for adding. Some minor updates:
| // The 256-bit internal state is four u64 lanes {v0, v1, mul0, mul1}, mixed in | ||
| // 128-bit halves (matching the reference SSE4.1 code), so this needs only | ||
| // Vec128 and works even on 128-bit-only targets. It does not yet use wider | ||
| // vectors when available; a 256-bit specialization is a possible follow-up. |
There was a problem hiding this comment.
Yes, a 256-bit specialization would be great. FYI we are planning on soon adding a 256-bit vector, at least if !HWY_HAVE_SCALABLE. That can already be relied upon if HWY_MIN_BYTES >= 32, and we can emulate this very similarly to the HWY_WASM_EMU256 target, but it seems difficult for SVE/RVV.
There was a problem hiding this comment.
Good to know — I'll leave the 128-bit-halves version as the baseline and can follow up with a HWY_MIN_BYTES >= 32 / !HWY_HAVE_SCALABLE path once the 256-bit vector lands.
|
|
||
| HWY_INLINE void UpdatePacket(const uint8_t* HWY_RESTRICT packet) { | ||
| const D64 d; | ||
| V64 lo = LoadU(d, reinterpret_cast<const uint64_t*>(packet) + 0); |
There was a problem hiding this comment.
HWY_RCAST_ALIGNED(const uint64_t*, packet) is preferred to placate the compiler.
There was a problem hiding this comment.
Switched UpdatePacket to load bytes directly (BitCast(d, LoadU(d8, packet))), so there's no uint64_t* cast at all — the whole-packet path is fed arbitrarily-aligned bytes + i from the caller, so I avoided asserting alignment there. 32b1552.
There was a problem hiding this comment.
Oh right, that's a better fix.
jan-wassenberg
left a comment
There was a problem hiding this comment.
Just noticed we have two build failures. One is werror:
highwayhash_test.cc:301:61: error: implicit conversion changes signedness: 'size_t' (aka 'unsigned long') to 'difference_type' (aka 'long') [-Werror,-Wsign-conversion]
301 | std::vector<uint8_t> data2(data.begin(), data.begin() + n);
The other is that the implementation must be wrapped in #if HWY_TARGET != HWY_SCALAR, because that deprecated target does not support some of the required ops.
|
I will make the code pass the tests so we can merge it, no comments at the code right? |
|
Yes, code LGTM, only remaining concern is the failing tests/actions. |
|
Updates LGTM. We still have issues with the RVV build and I've just realized why, sorry I didn't see this before. What we can do is instead store the state as uint64_t[], and Load/Store during the update. (We'll also require HWY_ALIGN on the class then.) Often the compiler is able to optimize out most of the excess load/store. Also, Vec128 is undocumented - an implementation detail that's often available, but not on RVV. What is always available is Full128, and we can use |
|
CI is green now (23 checks, 0 failures) at 574eb69. The multi-target fallout from the first push is resolved: SCALAR + the scalable targets (RVV/SVE, whose vectors can't be struct members) fall back to a HWY_DASSERT stub with the same guard |
|
At e8e764f every substantive check is green — x86 (all std versions), NEON/armv7, ppc64/ppc64le/s390x (incl. the big-endian runtime path), riscv64 (clang + gcc), loongarch, bazel, and The two remaining red X's — Summary of what changed since your review: SCALAR + the scalable targets (RVV/SVE, whose vectors can't be struct fields) fall back to a |
|
Update on the one red check — |
|
Nice :) I agree the tests are now fine. It seems like a pity to exclude SVE and RVV entirely - those could work, if we just refactor the state class to store uint64_t[] arrays instead of V64 vectors, and add Load/Store. Are you worried about perf implications? That would be understandable, seems like we could either disprove that via benchmark, or keep a second codepath using the uint64_t[] plus the original vector code. |
Per review: detail::State now holds `HWY_ALIGN uint64_t half[8][2]` (the eight 16-byte halves) instead of eight V64 members. Each round loads it into vectors, mixes, and stores it back. This drops the !HWY_HAVE_SCALABLE / !HWY_TARGET_IS_SVE exclusion - only HWY_SCALAR (1 lane) is still unsupported. TestGolden / TestLongInputs now run on the scalable targets too (previously stubbed). No perf change: the compiler keeps the array in registers (SROA). A/B on a 1 MiB buffer, clang 22 -O2: SSSE3 2.61 -> 2.62 GB/s, AVX2 2.93 -> 3.07 GB/s (within run-to-run noise), and objdump of the non-inlined kernel shows zero stack vector moves in either form. highwayhash_test still passes on AVX3/AVX2/SSE4/SSSE3/SSE2/EMU128 locally; strict -Werror and clang-format clean.
|
Done — SVE and RVV are enabled now.
No perf regression — the compiler keeps the array in registers (SROA), so
(the spread between runs is ~3%, so these are equal.)
The single unified codepath means no second implementation to keep in sync. |
jan-wassenberg
left a comment
There was a problem hiding this comment.
Nice, glad to hear the compiler optimizes out the loads/stores successfully, thanks for updating.
Resolves #3246.
Re-implements HighwayHash (https://arxiv.org/abs/1612.06257) with Highway ops
so a single source returns the frozen 64/128/256-bit golden values on every
target, rather than the reference repo's separate hand-written SSE4.1 / AVX2 /
NEON / VSX versions.
Approach
The 256-bit state
{v0, v1, mul0, mul1}(four u64 lanes) is mixed in 128-bithalves, exactly like the reference
HHStateSSE41. That keeps everything inVec128, so it also works on 128-bit-only targets (and EMU128). Op mapping:_mm_shuffle_epi32(v, 2,3,0,1)(swap u64 halves)RotateRight<32>_mm_mul_epu32MulEvenon the u32 view_mm_shuffle_epi8ZipperMergeTableLookupByteswith a fixed 16-byte indexRotate32By(length padding)RotateLeftSame<<1/<<2in the x^128+x^2+x reductionShiftRight+ShiftLeftBytes<8>Message bytes are little-endian; big-endian targets
ReverseLaneByteson load.The 1..31-byte remainder builds the padded packet with the frozen length-padding
layout (the
& 16split and the "unordered" 3-byte pack) in scalar code, thenfeeds it through the vector
Update, so no masked loads / lane inserts areneeded.
Public API:
HighwayHash64/128/256(key, bytes, size).Not yet specialized for wider vectors — a 256-bit-native path (one
Vec256perstate vector on AVX2/AVX3/SVE256+) is a natural follow-up.
Tests
highwayhash_test.cchashes every length in[0, 64](message = bytes0..n-1, key{0x0706..0100, ...}) against the frozen golden tables copiedfrom
google/highwayhashfor all three output widths, plus multi-packet inputsand a one-bit-change check. Passes on AVX2 / SSE4 / SSSE3 / SSE2 / EMU128
locally.
Wired into CMake, Bazel
BUILDandhwy_tests.bzl(new:highwayhashtarget). clang-format clean.