Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Implement multi-word division for BigInt #387

Merged
merged 1 commit into from
May 26, 2022
Merged

Conversation

ellie-idb
Copy link
Member

@ellie-idb ellie-idb commented Dec 16, 2021

This PR implements a version of Algorithm D (specified by Donald Knuth in TOACP Vol 2. 4.3, C implementation by Henry S. Warren Jr.), which aims to implement traditional "school-book" division for arbitrarily long numbers.

This is part of the process of getting #373 merged.

Status

  • Basic un-typed implementation
  • Implement in UInt
  • Implement in BigInt

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2021

Codecov Report

Merging #387 (d1ef009) into master (05abcc5) will increase coverage by 0.10%.
The diff coverage is 96.82%.

❗ Current head d1ef009 differs from pull request most recent head b9a6aa8. Consider uploading reports for the commit b9a6aa8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
+ Coverage   92.86%   92.97%   +0.10%     
==========================================
  Files          65       66       +1     
  Lines       15970    16303     +333     
==========================================
+ Hits        14830    15157     +327     
- Misses       1140     1146       +6     
Impacted Files Coverage Δ
source/mir/serde.d 72.50% <ø> (ø)
source/mir/series.d 94.69% <ø> (ø)
source/mir/numeric.d 92.89% <89.47%> (-0.48%) ⬇️
source/mir/format.d 95.83% <92.30%> (+0.12%) ⬆️
source/mir/bignum/integer.d 91.71% <97.67%> (+4.01%) ⬆️
source/mir/algebraic_alias/transform.d 100.00% <100.00%> (ø)
source/mir/bignum/fixed.d 95.90% <100.00%> (+1.65%) ⬆️
source/mir/bignum/low_level_view.d 94.10% <100.00%> (+0.13%) ⬆️
source/mir/math/stat.d 100.00% <100.00%> (ø)
source/mir/ndslice/fuse.d 86.07% <100.00%> (+0.54%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05abcc5...b9a6aa8. Read the comment docs.

@ellie-idb
Copy link
Member Author

@9il opCmp for BigUIntView fails if the coefficients don't exactly match, which means that a line like this will fail:

auto b = BigInt!4.fromHexString("c39b18a9f06fd8e962d99935cea0707f79a222050aaeaaaed17feb7aa76999d7");

auto rem = b /= UInt!128.fromHexString("f79a222050aaeaaa417fa25a2ac93291"); 

// fails
// b is BigIntView!(ulong, WordEndian.little)(BigUIntView!(ulong, WordEndian.little)([8989662390899871995, 14572942669551396987, 0, 0]), false)
// rhs is BigIntView!(ulong, WordEndian.little)(BigUIntView!(ulong, WordEndian.little)([8989662390899871995, 14572942669551396987]), false)
assert(b == BigInt!4.fromHexString("ca3d7e25aebe687b7cc1b250b44690fb"));
assert(rem == UInt!128.fromHexString("bf4c87424431d21563f23b1fc00d75ac"));

Should I automatically normalize the quotient view after dividing? Or would you be fine with replacing BigUIntView.opCmp with:

    ///
    bool opEquals(scope BigUIntView!(const W, endian) rhs)
        const @safe pure nothrow @nogc scope
    {
        return this.normalized.coefficients == rhs.normalized.coefficients;
    }


// Similarly here, this is specified as qhat*v[n-2] > rhat*b + u[j+n-2]
// but we can just get away with a shift instead.
while (qhat >= b || qhat*v[n-2] > (rhat << shift) + u[j+n-2])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just qhat > uint.max and remove b

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplifying the while statement to just while (qhat > uint.max) causes test failure, but I'll pull out the b variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean qhat >= b => qhat > uint.max . The other part of condition is OK

source/mir/bignum/fixed.d Outdated Show resolved Hide resolved
@9il
Copy link
Member

9il commented Dec 23, 2021

Should I automatically normalize the quotient view after dividing? Or would you be fine with replacing BigUIntView.opCmp with:

Yes, it should be normalized inside divm. You can pass the views by reference if needed. The same is true for the remainder.

@ellie-idb ellie-idb marked this pull request as ready for review December 27, 2021 18:15
source/mir/bignum/fixed.d Outdated Show resolved Hide resolved
auto rem = a /= b;
assert(a == UInt!128.fromHexString("103e70bb237983337"));
assert(rem == UInt!64.fromHexString("4932c5448b170cb3"));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we need %=, /, and % operations. And standalone divRem method that performs both division and remainder for Uints. All /=, %=, /, and % can call it instead of the divm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I got this implemented in UInt -- should it also be implemented in BigInt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No

@ellie-idb
Copy link
Member Author

@9il able to review?

dub.sdl Outdated Show resolved Hide resolved
source/mir/bignum/fixed.d Outdated Show resolved Hide resolved
source/mir/bignum/fixed.d Outdated Show resolved Hide resolved
source/mir/bignum/fixed.d Outdated Show resolved Hide resolved
auto rem = a /= b;
assert(a == UInt!128.fromHexString("103e70bb237983337"));
assert(rem == UInt!64.fromHexString("4932c5448b170cb3"));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No

source/mir/bignum/low_level_view.d Outdated Show resolved Hide resolved
source/mir/bignum/low_level_view.d Outdated Show resolved Hide resolved
source/mir/bignum/integer.d Outdated Show resolved Hide resolved
source/mir/bignum/integer.d Outdated Show resolved Hide resolved
source/mir/bignum/integer.d Outdated Show resolved Hide resolved
@ellie-idb ellie-idb force-pushed the impl-bigint-division branch 3 times, most recently from f5b98e5 to 313771b Compare January 24, 2022 19:42
@ellie-idb ellie-idb requested a review from 9il January 31, 2022 20:35
Copy link
Member

@9il 9il left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still pending comments.

source/mir/bignum/integer.d Outdated Show resolved Hide resolved
source/mir/bignum/integer.d Outdated Show resolved Hide resolved
@ellie-idb
Copy link
Member Author

FYI: This is ready for further review @9il

@9il 9il merged commit d930eca into master May 26, 2022
@9il 9il deleted the impl-bigint-division branch May 26, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants