Conversation
Accumulate, Broadcast and the two Get*RotationIndices helpers advance the rotation stride by logbStep = bit_width(bStep) - 1 and issue bStep - 1 hoisted rotations per round, which is only correct when bStep is a power of two >= 2 (as the header already states). bStep == 1 makes logbStep 0, so the round loop `s <<= logbStep` never terminates; a non-power-of-two bStep such as 3 sums some shifts twice and returns a wrong result without any error. Validate bStep once, in one helper, and throw std::invalid_argument (the exception type the library already uses for bad arguments). The cascade variant's silent early return for bStep <= 1 becomes the same error. No change for valid arguments. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
Accumulate,Broadcastand the twoGet*RotationIndiceshelpers advance the rotation stride bylogbStep = bit_width(bStep) - 1and issuebStep - 1hoisted rotations per round. That is only correct whenbStepis a power of two >= 2, which the header already states. Two invalid values currently fail silently:bStep == 1giveslogbStep == 0, so the round loops <<= logbStepnever terminates. We lost several hours to this in an extraction stage before finding it.bStepthat is not a power of two (e.g. 3) sums some shifts twice and returns a wrong result without any error.This PR validates
bSteponce, in a small helper, and throwsstd::invalid_argumentwith the offending value (the exception type the library already uses for bad arguments). The cascade variant's silent early return forbStep <= 1becomes the same error; itsstartFactor/sizeguards are unchanged. Nothing changes for valid arguments: the library's own callers use 2 or 4 (bootstrap precomputation, bert-tiny example). A host-only gtest covers the accepted and rejected values.Tested on v2.1.3 (786c760), CUDA 13.0, H200: full build with
FIDESLIB_COMPILE_TESTS=ON, new test passes. The commit also applies cleanly onOpenFHECompatTests.Fable 5.1 on behalf of Seyfal