fix(compute): handle pure-Go comparison output offsets#956
fix(compute): handle pure-Go comparison output offsets#956fallintoplace wants to merge 1 commit into
Conversation
5c80111 to
632c218
Compare
zeroshade
left a comment
There was a problem hiding this comment.
The pure-Go fix here is correct and nicely tested — handling the leading prefix := offset % 8 sub-batch before the aligned batch loop is exactly right, and the temp-buffer handling is clean.
The concern is that this only fixes the pure-Go path. On the default amd64 build the comparison kernels dispatch to the generated SIMD assembly (scalar_comparison_avx2_amd64.s / scalar_comparison_sse4_amd64.s, generated from _lib/scalar_comparison.cc), which still carries the original output-offset bug:
num_batches = length / kBatchSizeis computed from the originallengthbefore any prefix handling, and the tail loop iterates over the originallength.- With a non-zero output offset (e.g.
length=1, offset%8==7) the SIMD path can read past the input and/or corrupt bits beyond the intended output span.
Because the new tests call the pure-Go helpers directly, they pass green even though the default amd64 dispatch is still broken.
Requested changes before merge:
- Apply the same prefix/offset handling in
_lib/scalar_comparison.ccand regeneratescalar_comparison_avx2_amd64.s/scalar_comparison_sse4_amd64.s. - Add a kernel-level test that exercises the real dispatch (not the pure-Go helper directly) with a non-zero output offset, so the amd64 SIMD path is actually covered.
| ) | ||
|
|
||
| tmpOutSlice := tmpOutput[:] | ||
| if prefix := offset % 8; prefix != 0 { |
There was a problem hiding this comment.
This prefix handling is correct for the pure-Go path — but the same fix needs to land in _lib/scalar_comparison.cc and the regenerated *_amd64.s, which is what the default amd64 build actually dispatches to. On amd64, num_batches/tail are still derived from the original length, so a non-zero output offset still over-reads / corrupts bits. See the main review comment for the full ask.
Rationale for this change
The pure-Go primitive comparison path assumes that a partial output byte always has enough input values to fill it. It also calculates the batch count before consuming that prefix. Short inputs can panic, while longer inputs can process the wrong ranges when the output starts at a non-byte-aligned offset.
This path is used by
noasmbuilds and when the SIMD implementations are unavailable.What changes are included in this PR?
Are these changes tested?
Yes. The compute subtree tests, focused race tests, 100 repeated regression runs, and linux/amd64
noasmcompilation pass. The regression also verifies that bits outside the output range remain unchanged.Are there any user-facing changes?
Pure-Go numeric and decimal comparisons no longer panic or produce incorrect output for non-byte-aligned result offsets. There is no API change.