-
Notifications
You must be signed in to change notification settings - Fork 30
Karatsuba multiplication #95
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
bdabfe8
244a6a8
f2a48e2
9e4efd6
f3bcc9b
bc118cb
438a26e
5dd2512
dc69b3c
fd4d321
bb05ee5
1327726
d7bfc9f
290a1d1
334537a
463ece4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -64,6 +64,7 @@ func initBigInt*(val: BigInt): BigInt = | |||||||||||||||||
const | ||||||||||||||||||
zero = initBigInt(0) | ||||||||||||||||||
one = initBigInt(1) | ||||||||||||||||||
karatsubaTreshold = 10 | ||||||||||||||||||
|
||||||||||||||||||
func isZero(a: BigInt): bool {.inline.} = | ||||||||||||||||||
for i in countdown(a.limbs.high, 0): | ||||||||||||||||||
|
@@ -388,7 +389,6 @@ template `-=`*(a: var BigInt, b: BigInt) = | |||||||||||||||||
assert a == 3.initBigInt | ||||||||||||||||||
a = a - b | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
func unsignedMultiplication(a: var BigInt, b, c: BigInt) {.inline.} = | ||||||||||||||||||
# always called with bl >= cl | ||||||||||||||||||
let | ||||||||||||||||||
|
@@ -418,6 +418,26 @@ func unsignedMultiplication(a: var BigInt, b, c: BigInt) {.inline.} = | |||||||||||||||||
inc pos | ||||||||||||||||||
normalize(a) | ||||||||||||||||||
|
||||||||||||||||||
func scalarMultiplication(a: var BigInt, b: BigInt, c: uint32) {.inline.} = | ||||||||||||||||||
# always called with bl >= cl | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment needs to be removed, since |
||||||||||||||||||
let | ||||||||||||||||||
bl = b.limbs.len | ||||||||||||||||||
a.limbs.setLen(bl + 1) | ||||||||||||||||||
var tmp = 0'u64 | ||||||||||||||||||
|
||||||||||||||||||
for i in 0 ..< bl: | ||||||||||||||||||
tmp += uint64(b.limbs[i]) * uint64(c) | ||||||||||||||||||
a.limbs[i] = uint32(tmp and uint32.high) | ||||||||||||||||||
tmp = tmp shr 32 | ||||||||||||||||||
|
||||||||||||||||||
a.limbs[bl] = uint32(tmp) | ||||||||||||||||||
normalize(a) | ||||||||||||||||||
dlesnoff marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
|
||||||||||||||||||
# forward declaration for use in `multiplication` | ||||||||||||||||||
func karatsubaMultiplication(a: var BigInt, b, c: BigInt) {.inline.} | ||||||||||||||||||
dlesnoff marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||
func `shl`*(x: BigInt, y: Natural): BigInt | ||||||||||||||||||
func `shr`*(x: BigInt, y: Natural): BigInt | ||||||||||||||||||
|
||||||||||||||||||
func multiplication(a: var BigInt, b, c: BigInt) = | ||||||||||||||||||
# a = b * c | ||||||||||||||||||
if b.isZero or c.isZero: | ||||||||||||||||||
|
@@ -428,11 +448,69 @@ func multiplication(a: var BigInt, b, c: BigInt) = | |||||||||||||||||
cl = c.limbs.len | ||||||||||||||||||
|
||||||||||||||||||
if cl > bl: | ||||||||||||||||||
unsignedMultiplication(a, c, b) | ||||||||||||||||||
if bl <= karatsubaTreshold: | ||||||||||||||||||
karatsubaMultiplication(a, c, b) | ||||||||||||||||||
else: | ||||||||||||||||||
unsignedMultiplication(a, c, b) | ||||||||||||||||||
|
if bl <= karatsubaTreshold: | |
karatsubaMultiplication(a, c, b) | |
else: | |
unsignedMultiplication(a, c, b) | |
if bl > karatsubaTreshold: | |
karatsubaMultiplication(a, c, b) | |
else: | |
unsignedMultiplication(a, c, b) |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if cl <= karatsubaTreshold: | |
karatsubaMultiplication(a, b, c) | |
else: | |
unsignedMultiplication(a, b, c) | |
if cl > karatsubaTreshold: | |
karatsubaMultiplication(a, b, c) | |
else: | |
unsignedMultiplication(a, b, c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if bl == 1: | |
# base case : multiply the only limb with each limb of second term | |
scalarMultiplication(a, c, b.limbs[0]) | |
return | |
if cl == 1: | |
scalarMultiplication(a, b, c.limbs[0]) | |
return |
This is already done in unsignedMultiplication
afaict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to reduce the number of operations if we know there is only one limb.
We do not have to do a whole for loop.
But you are right, we can do a scalarMultiplication with unsignedMultiplication.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes more sense to move this logic into unsignedMultiplication
. Then we avoid checking this twice when calling karatsubaMultiplication
from multiplication
and we can call unsignedMultiplication
inside karatsubaMultiplication
(instead of karatsubaMultiplication
itself).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed many bugs in the version you just reviewed, I am sorry I should have warned you, there will be many changes to this version.
I made karatsubaMultiplication and multiplication completely independent to test them independently and compare obtained results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine, I didn't expect this to be the final version anyway. As I said, I haven't looked at karatsubaMultiplication
in depth yet, since I want to better understand the algorithm first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When c.limbs.len
is smaller than half of b.limbs.len
, k
is larger than c.limbs.len
and low_c.limbs = c.limbs[0 .. k-1]
cause IndexDefect.
It would be better to add tests that multiply a large value by a small value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can simply change n to the min of bl and cl in this case.
I want to add these tests, but I am waiting for my other PRs to be merged. In these, I have added random generation and benchmarks. I am especially waiting for #112 . With initRandomBigInt, I will be able to generate bigints of a specific size for tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These all create new seq
s, which isn't very efficient. Perhaps we should use openArray[uint32]
instead (then this can use toOpenArray
for O(1) slicing) or make our own "sliceable seq" to avoid copies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did not thougt about this. I used seq because I expect to join the results into a seq after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@konsumlamm openArray are only used for procs arguments, when we want either a seq or an array as argument.
Can't we use some pointers here ? Do we have to create a new structure ?
Internal structure is a seq, if we use another structure, we would have to make a copy anyway or change the internal structure.
We can not use arrays neither, since we do not know the size at compile time. (It depends on the variable k, which value is known at runtime).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An openArray
is basically a pointer and a length, it doesn't create a copy. Anything that doesn't create a copy but just modifies indices/pointers should be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see how we can call multiplication then on those pointers, without making a multiplication directly on arrays.
We need to initialize BigInts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These all create new
seq
s, which isn't very efficient. Perhaps we should useopenArray[uint32]
instead (then this can usetoOpenArray
for O(1) slicing) or make our own "sliceable seq" to avoid copies.
The algorithm wants the value of the polynomial corresponding to each parts of the slice.
The best way so far that I conceive is to modify the BigInt's limbs field from:
type
BigInt* = object
limbs: seq[uint32]
isNegative: bool
to
type
BigInt* = object
limbs: ref seq[uint32]
isNegative: bool
Otherwise, we will have to reimplement addition, subtraction and base case multiplication for another container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seqs are openarrays, if you implement for openarrays, it's implemented for seq and arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to change the whole code.
Addition, subtraction and multiplication take bigints with a sequence parameter as input.
For the Karatsuba algorithm as well as some others, we need to manipulate these sequences through a pointer and get a pointer for parts of the sequence.
We also need to operate on those slices of the sequence, i.e. get the value associated with each part of the slice, and add, subtract, and multiply these slices.
That's why we need a seq-like container with the possibility of getting a reference to each value of the seq.
None of the openarray types enables this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @konsumlamm said, I think openArray and toOpenArray
is good enough to implement karatsuba multiplication.
You can write recursive procedure like this using openArray and toOpenArray
:
proc sum(x: openArray[int]): int =
case x.len:
of 0:
0
of 1:
x[0]
of 2:
x[0] + x[1]
else:
let mid = x.len div 2
sum(toOpenArray(x, 0, mid - 1)) + sum(toOpenArray(x, mid, x.high))
echo sum([1, 2, 3, 4, 5])
This is a part of karatsubaMultiplication
in your current PR:
var
low_b, high_b, low_c, high_c: BigInt
# Decompose `b` and `c` in two parts of (almost) equal length
low_b.limbs = b.limbs[0 .. k-1]
high_b.limbs = b.limbs[k .. ^1]
low_c.limbs = c.limbs[0 .. k-1]
high_c.limbs = c.limbs[k .. ^1]
# subtractive version of Karatsuba's algorithm to limit carry handling
var lowProduct, highProduct, add3, add4, add5, middleTerm: BigInt = zero
multiplication(lowProduct, low_b, low_c)
multiplication(highProduct, high_b, high_c)
add3 = low_b - high_b
add4 = high_c - low_c
Above code would be written using toOpenArray
like:
# Decompose `b` and `c` in two parts of (almost) equal length
template low_b = toOpenArray(b.limbs, 0, k - 1)
template high_b = toOpenArray(b.limbs, k, b.limbs.high)
template low_c = toOpenArray(c.limbs, 0, k - 1)
template high_c = toOpenArray(c.limbs, k, c.limbs.high)
# subtractive version of Karatsuba's algorithm to limit carry handling
var lowProduct, highProduct, add3, add4, add5, middleTerm: BigInt = zero
multiplication(lowProduct, low_b, low_c)
multiplication(highProduct, high_b, high_c)
# This code requires subtraction proc that takes 2 `openArray`
add3 = low_b - high_b
add4 = high_c - low_c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the time taken and the detailed changes.
# This code requires subtraction proc that takes 2 `openArray`
I just want to point out that scalarMultiplication and unsignedMultiplication will also have to take openArrays:
if bl == 1:
scalarMultiplication(a, c, b.limbs[0]) # b and c are openArrays
a.isNegative = b.isNegative xor c.isNegative
return
...
if bl < karatsubaThreshold:
if cl <= bl:
unsignedMultiplication(a, b, c) # b and c are openArrays here
else:
unsignedMultiplication(a, c, b) # same
a.isNegative = b.isNegative xor c.isNegative
return
...
I can make a multiplication(a: var Bigint, b, c: openArray[uint32]) =
proc, and avoid to rewrite addition and shr for openArrays.
I read the unsignedMultiplication and scalarMultiplication proc algorithms again and they effectively don't need parameters to be BigInts.
The substract
proc will be quite delicate to convert for OpenArray parameters though.
I will look into it.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
low_b
and low_c
aren't necessarily normalized afaict, so that may cause problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be the problem I encounter at compilation time. I have not done any test when the operands does not have the same limbs sequence’s length.
Uh oh!
There was an error while loading. Please reload this page.