Repository navigation
Fix quotient digit estimate in division (fixes #123) - #151
Merged
Merged
Conversation
When the top limb of the running remainder equals the top limb of the normalized divisor, vv div wm1 >= 2^32 and the two-limb correction test cannot always bring the estimate back below 2^32 (it stops at exactly 2^32 when the next limbs are equal too), tripping `assert q1 <= uint32.high`. Restore the clamp from Knuth, TAOCP vol. 2, Algorithm D, step D3: if the top limbs are equal, set q1 = 2^32 - 1 and r1 = next limb + wm1, and skip the correction test once r1 >= 2^32. Adds regression tests with the numbers from the issue and minimal cases.
Contributor
Author
|
posting also here the initial conversation with the fix I shared in the issue thread https://claude.ai/share/84e37c1a-fc9c-4117-85a6-e2a975247ea0 with respect to the original patch, this PR fix adds an additional test case. Also of interest might be a recent article that actually finds a bug in Knuth's original algorithm (and collects the check!). The bug apply only to odd bases but the article is very interesting to have a better understanding and appreciation of the algo https://kolja.rs/algorithm-d/ |
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
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.
fix #123
Fix was produced by Claude, tests are added, Claude made additional fuzzing (not reported here). My effort was to review, understand and validate.
Here is a quick explanation from first principles:
In the details below (claude generated) you might also want to know more about knuth algorithm (aka Algorithm D):
details
When the top limb of the running remainder equals the top limb of the normalized divisor, vv div wm1 >= 2^32 and the two-limb correction test cannot always bring the estimate back below 2^32 (it stops at exactly 2^32 when the next limbs are equal too), tripping
assert q1 <= uint32.high.Restore the clamp from Knuth, TAOCP vol. 2, Algorithm D, step D3: if the top limbs are equal, set q1 = 2^32 - 1 and r1 = next limb + wm1, and skip the correction test once r1 >= 2^32.
Adds regression tests with the numbers from the issue and minimal cases.