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

Added widemul for u256. #3108

Merged
merged 1 commit into from
May 10, 2023
Merged

Added widemul for u256. #3108

merged 1 commit into from
May 10, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented May 10, 2023

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.


This change is Reviewable

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi and @spapinistarkware)


corelib/src/integer.cairo line 1092 at r1 (raw file):

#[inline(always)]
fn u256_wide_mul_add_limb2_helper(ref value: u512, limb: u128) {

doc + why needed specifically for limb2...


corelib/src/integer.cairo line 1099 at r1 (raw file):

        Result::Err(v) => {
            value.limb2 = v;
            value.limb3 += 1;

what if limb3 overflows now?
I know it should not happen when these are called from u256_wide_mul, but this is a public function. Either handle the case, or document that it doesn't handle limb3 overflow and that it's only to be used by u256_wide_mul, the way it is used now.


corelib/src/integer.cairo line 1105 at r1 (raw file):

#[inline(always)]
fn u256_wide_mul_add_limb1_helper(ref value: u512, limb: u128) {

same


corelib/src/integer.cairo line 1124 at r1 (raw file):

    u256_wide_mul_add_limb1_helper(ref result, limb1_part);
    u256_wide_mul_add_limb2_helper(ref result, limb2_part);
    let (limb2_part, limb1_part) = u128_wide_mul(a.high, b.low);

As you call each helper twice, it has the potential to add 1 (to the next limb) twice. To easily improve it a bit, you can change the helper to return the addition you need to the next limb (either a bool or a u128). Then you call the limb1 helper twice and aggregate the addition to limb2 to add it at once. Same for limb2 and the addition to limb3 (here you actually call the limb2 helper 3 times (once again with the overflow from limb1))


corelib/src/test/integer_test.cairo line 681 at r1 (raw file):

#[test]
fn test_u256_wide_mul() {
    assert(u256_wide_mul(0, 0) == u512 { limb0: 0, limb1: 0, limb2: 0, limb3: 0 }, '0 * 0 == 0');

'0 * 0 != 0'.
This is the error message in case of assertion failure.

Also below: 'long calculation failed'

@orizi orizi force-pushed the pr/orizi/mulmodu256/7c544b0c branch from 72edd72 to 1b94dfc Compare May 10, 2023 08:01
Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware and @yuvalsw)


corelib/src/integer.cairo line 1092 at r1 (raw file):

Previously, yuvalsw wrote…

doc + why needed specifically for limb2...

Done.
It is needed since i call it.


corelib/src/integer.cairo line 1099 at r1 (raw file):

Previously, yuvalsw wrote…

what if limb3 overflows now?
I know it should not happen when these are called from u256_wide_mul, but this is a public function. Either handle the case, or document that it doesn't handle limb3 overflow and that it's only to be used by u256_wide_mul, the way it is used now.

added doc.


corelib/src/integer.cairo line 1124 at r1 (raw file):

Previously, yuvalsw wrote…

As you call each helper twice, it has the potential to add 1 (to the next limb) twice. To easily improve it a bit, you can change the helper to return the addition you need to the next limb (either a bool or a u128). Then you call the limb1 helper twice and aggregate the addition to limb2 to add it at once. Same for limb2 and the addition to limb3 (here you actually call the limb2 helper 3 times (once again with the overflow from limb1))

this isn't as simple as you claim here - if it is - please give a code suggestion.


corelib/src/test/integer_test.cairo line 681 at r1 (raw file):

Previously, yuvalsw wrote…

'0 * 0 != 0'.
This is the error message in case of assertion failure.

Also below: 'long calculation failed'

we don't have that many chars for error - my main aim is to easily find the the failed assertion, while being economic with the err msg - suggestions for better assertion strings?

@orizi orizi force-pushed the pr/orizi/mulmodu256/7c544b0c branch 2 times, most recently from 241f5b6 to 3a0280e Compare May 10, 2023 08:54
Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi and @spapinistarkware)


corelib/src/integer.cairo line 1124 at r1 (raw file):

Previously, orizi wrote…

this isn't as simple as you claim here - if it is - please give a code suggestion.

#[derive(Copy, Drop, PartialEq, Serde)]
struct u512 {
limb0: u128,
limb1: u128,
limb2: u128,
limb3: u128,
}

#[inline(always)]
fn u256_wide_mul_add_limb2_helper(ref value: u512, limb: u128) -> u128 nopanic {
match u128_overflowing_add(value.limb2, limb) {
Result::Ok(v) => {
value.limb2 = v;
0
},
Result::Err(v) => {
value.limb2 = v;
1
},
}
}

#[inline(always)]
fn u256_wide_mul_add_limb1_helper(ref value: u512, limb: u128) -> u128 nopanic {
match u128_overflowing_add(value.limb1, limb) {
Result::Ok(v) => {
value.limb1 = v;
0
},
Result::Err(v) => {
value.limb1 = v;
1
},
}
}

fn u256_wide_mul(a: u256, b: u256) -> u512 nopanic {
let (limb1, limb0) = u128_wide_mul(a.low, b.low);
let (limb3, limb2) = u128_wide_mul(a.high, b.high);
let mut result = u512 { limb0, limb1, limb2, limb3 };

let (ab_limb2_part, ab_limb1_part) = u128_wide_mul(a.low, b.high);
let (ba_limb2_part, ba_limb1_part) = u128_wide_mul(b.low, a.high);

let limb1_overflow1 = u256_wide_mul_add_limb1_helper(ref result, ab_limb1_part);
let limb1_overflow2 = u256_wide_mul_add_limb1_helper(ref result, ba_limb1_part);
// No overflow possible in this addition since both operands are 0/1.
let limb1_overflow = u128_wrapping_add(limb1_overflow1, limb1_overflow2);

let limb2_overflow1 = u256_wide_mul_add_limb2_helper(ref result, ab_limb2_part);
let limb2_overflow2 = u256_wide_mul_add_limb2_helper(ref result, ba_limb2_part);
let limb2_overflow3 = u256_wide_mul_add_limb2_helper(ref result, limb1_overflow);
// No overflow possible in this additions since all operands are 0/1.
let limb2_overflow = u128_wrapping_add(
    u128_wrapping_add(limb2_overflow1, limb2_overflow2), limb2_overflow3
);

// No overflow possible since the product of 2 u256s can't overflow a u512.
value.limb3 = u128_wrapping_add(value.limb3, limb2_overflow);

result

}


corelib/src/test/integer_test.cairo line 681 at r1 (raw file):

Previously, orizi wrote…

we don't have that many chars for error - my main aim is to easily find the the failed assertion, while being economic with the err msg - suggestions for better assertion strings?

We have 31 chars, don't we?
My suggestions are less than 31:
'0 * 0 != 0'
'long calculation failed'

commit-id:7c544b0c
@orizi orizi force-pushed the pr/orizi/mulmodu256/7c544b0c branch from 3a0280e to 41a30a9 Compare May 10, 2023 11:43
Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware and @yuvalsw)


corelib/src/integer.cairo line 1124 at r1 (raw file):

Previously, yuvalsw wrote…

#[derive(Copy, Drop, PartialEq, Serde)]
struct u512 {
limb0: u128,
limb1: u128,
limb2: u128,
limb3: u128,
}

#[inline(always)]
fn u256_wide_mul_add_limb2_helper(ref value: u512, limb: u128) -> u128 nopanic {
match u128_overflowing_add(value.limb2, limb) {
Result::Ok(v) => {
value.limb2 = v;
0
},
Result::Err(v) => {
value.limb2 = v;
1
},
}
}

#[inline(always)]
fn u256_wide_mul_add_limb1_helper(ref value: u512, limb: u128) -> u128 nopanic {
match u128_overflowing_add(value.limb1, limb) {
Result::Ok(v) => {
value.limb1 = v;
0
},
Result::Err(v) => {
value.limb1 = v;
1
},
}
}

fn u256_wide_mul(a: u256, b: u256) -> u512 nopanic {
let (limb1, limb0) = u128_wide_mul(a.low, b.low);
let (limb3, limb2) = u128_wide_mul(a.high, b.high);
let mut result = u512 { limb0, limb1, limb2, limb3 };

let (ab_limb2_part, ab_limb1_part) = u128_wide_mul(a.low, b.high);
let (ba_limb2_part, ba_limb1_part) = u128_wide_mul(b.low, a.high);

let limb1_overflow1 = u256_wide_mul_add_limb1_helper(ref result, ab_limb1_part);
let limb1_overflow2 = u256_wide_mul_add_limb1_helper(ref result, ba_limb1_part);
// No overflow possible in this addition since both operands are 0/1.
let limb1_overflow = u128_wrapping_add(limb1_overflow1, limb1_overflow2);

let limb2_overflow1 = u256_wide_mul_add_limb2_helper(ref result, ab_limb2_part);
let limb2_overflow2 = u256_wide_mul_add_limb2_helper(ref result, ba_limb2_part);
let limb2_overflow3 = u256_wide_mul_add_limb2_helper(ref result, limb1_overflow);
// No overflow possible in this additions since all operands are 0/1.
let limb2_overflow = u128_wrapping_add(
    u128_wrapping_add(limb2_overflow1, limb2_overflow2), limb2_overflow3
);

// No overflow possible since the product of 2 u256s can't overflow a u512.
value.limb3 = u128_wrapping_add(value.limb3, limb2_overflow);

result

}

Done.

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

@orizi orizi added this pull request to the merge queue May 10, 2023
Merged via the queue into main with commit 834df7e May 10, 2023
@orizi orizi deleted the pr/orizi/mulmodu256/7c544b0c branch May 22, 2023 06:09
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.

2 participants