Repository navigation
fix: honour BASE64_FORCE_AVX512 in base64_stream_encode_init - #163
Open
mvanslobbe wants to merge 1 commit into
Open
mvanslobbe wants to merge 1 commit into
mvanslobbe wants to merge 1 commit into
Conversation
BASE64_FORCE_AVX512 is bit 8, but the encoder init only re-runs codec selection for flags & 0xFF. Once a codec has been chosen, forcing AVX512 for encoding is silently ignored and the previous codec keeps running. The decoder init already uses 0xFFFF.
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
1 participant
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.
base64_stream_encode_initonly redoes codec selection whenflags & 0xFFis non-zero.BASE64_FORCE_AVX512is1 << 8, so once any codec has been selected, a forced AVX512 encode carries on using the previous codec.base64_stream_decode_inituses0xFFFFand is not affected. The mask dates from when AVX was the highest flag and was not widened when AVX512 was added in 98352df.Output is unaffected, since all codecs produce identical results, but the wrong codec runs. Callers using the default flags of
0never hit this; it only affects callers that force a codec. The visible consequence is in the test suite:test_streamingbuilds its reference withBASE64_FORCE_PLAIN, which switches the global codec to plain, and the followingbase64_stream_encode_init(&state, BASE64_FORCE_AVX512)keeps plain. The AVX512 streaming encode test has therefore never exercised the AVX512 encoder.Measured by wrapping
base64_stream_encode_avx512with a call counter and runningtest_base64under Intel SDE (-icl):All tests pass after the change, including AVX512 streaming encode at every block size.
test/benchmark.cis not affected, becausecodec_supported()selects the codec through a decode first.A shared mask macro for both init functions would stop this from recurring when the next flag is added; happy to add that if you'd prefer it over the minimal change.