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

u256 wide multiplication #2228

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

u256 wide multiplication #2228

wants to merge 32 commits into from

Conversation

Genya-Z
Copy link

@Genya-Z Genya-Z commented Feb 21, 2023

Adding u256_wide_mul and u128_to_64s.


This change is Reviewable

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 6 files at r2.
Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @Genya-Z)


corelib/test.cairo line 713 at r2 (raw file):

    assert(
	high == as_u256(130366876754661164843311819125622077435_u128,
			238310065584501807926300814868835410406_u128),

what are these numbers? used named variables please

Code quote:

    let(low,high)=integer::u256_wide_mul(as_u256(155419417030398358529415680970430503750_u128,
						 208595563450721923828867081157420252200_u128),
					 as_u256(285431191531813133557775306831253175872_u128,
						 43439607001498238463885217561246358334_u128));
    assert(
        low == as_u256(74008176751363765864810996693396202398_u128,
		       113989470359884732637183370837798242736_u128),
	'wide mul low'
    );
    assert(
	high == as_u256(130366876754661164843311819125622077435_u128,
			238310065584501807926300814868835410406_u128),
        'wide mul high'
    );

crates/cairo-lang-sierra/src/extensions/modules/uint128.rs line 166 at r2 (raw file):

        context: &dyn SignatureSpecializationContext,
    ) -> Result<LibfuncSignature, SpecializationError> {
        let ty = context.get_concrete_type(Uint128Type::id(), &[])?;

rename

Suggestion:

        let ty_u128 = context.get_concrete_type(Uint128Type::id(), &[])?;
        let ty_u64 = context.get_concrete_type(Uint64Type::id(), &[])?;

crates/cairo-lang-sierra-to-casm/src/invocations/uint128.rs line 218 at r2 (raw file):

    Ok(builder.build_from_casm_builder(
        casm_builder,
        [("Fallthrough", &[&[range_check], &[low], &[high]], None)],

why this order?

Code quote:

&[low], &[high]

docs/CODE_OF_CONDUCT.md line 1 at r2 (raw file):

# Contributor Covenant Code of Conduct

why is this deleted?

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware 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: 2 of 8 files reviewed, 5 unresolved discussions (waiting on @Genya-Z)


corelib/integer.cairo line 770 at r2 (raw file):

const HALF_SHIFT : felt = 18446744073709551616;//2^64;
//const HALF_SHIFTb  = u128_as_non_zero(18446744073709551616_u128);//2^64;
fn u256_wide_mul(a: u256, b: u256) -> (u256, u256) implicits(RangeCheck) {

deleted commented code lines throughout this function

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware 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 1 files at r4.
Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @Genya-Z)

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @Genya-Z)

Copy link
Author

@Genya-Z Genya-Z 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: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)


corelib/test.cairo line 713 at r2 (raw file):

Previously, dorimedini-starkware wrote…

what are these numbers? used named variables please

These are random integers I generated to test the multiplication routine.


crates/cairo-lang-sierra/src/extensions/modules/uint128.rs line 166 at r2 (raw file):

Previously, dorimedini-starkware wrote…

rename

Done.


crates/cairo-lang-sierra-to-casm/src/invocations/uint128.rs line 218 at r2 (raw file):

Previously, dorimedini-starkware wrote…

why this order?

It's how the function worked in the Cairo 0 version.

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 3 of 8 files reviewed, 3 unresolved discussions (waiting on @Genya-Z and @orizi)


corelib/test.cairo line 713 at r2 (raw file):

Previously, Genya-Z wrote…

These are random integers I generated to test the multiplication routine.

please add some edge cases with named constants


crates/cairo-lang-sierra-to-casm/src/invocations/uint128.rs line 218 at r2 (raw file):

Previously, Genya-Z wrote…

It's how the function worked in the Cairo 0 version.

@orizi WDYT?

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 8 files at r6, all commit messages.
Reviewable status: 4 of 10 files reviewed, 6 unresolved discussions (waiting on @Genya-Z and @orizi)


crates/cairo-lang-sierra/src/extensions/modules/uint128.rs line 28 at r6 (raw file):

        Operation(UintOperationLibfunc<Uint128Traits>),
        Divmod(UintDivmodLibfunc<Uint128Traits>),
	Split(Uint128ToUint64sLibfunc),

for spaces instead of tabs (everywhere)


corelib/integer.cairo line 0 at r6 (raw file):
why deleted?


corelib/test.cairo line 0 at r6 (raw file):
why deleted?

Copy link
Author

@Genya-Z Genya-Z 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: 4 of 10 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware and @orizi)


corelib/integer.cairo line 770 at r2 (raw file):

Previously, dorimedini-starkware wrote…

deleted commented code lines throughout this function

Done.


corelib/integer.cairo line at r6 (raw file):

Previously, dorimedini-starkware wrote…

why deleted?

It was moved into /src/ in the main branch.


corelib/test.cairo line 713 at r2 (raw file):

Previously, dorimedini-starkware wrote…

please add some edge cases with named constants

Done.

Copy link
Author

@Genya-Z Genya-Z 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: 3 of 10 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware and @orizi)


crates/cairo-lang-sierra/src/extensions/modules/uint128.rs line 28 at r6 (raw file):

Previously, dorimedini-starkware wrote…

for spaces instead of tabs (everywhere)

Done.

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware 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: 3 of 10 files reviewed, 5 unresolved discussions (waiting on @Genya-Z and @orizi)


crates/cairo-lang-sierra/src/extensions/modules/uint128.rs line 28 at r6 (raw file):

Previously, Genya-Z wrote…

Done.

image.png
nope :)
also, I meant four spaces instead of tabs

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware 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: 3 of 10 files reviewed, 5 unresolved discussions (waiting on @Genya-Z and @orizi)


crates/cairo-lang-sierra/src/extensions/modules/uint128.rs line 28 at r6 (raw file):

Previously, dorimedini-starkware wrote…

image.png
nope :)
also, I meant four spaces instead of tabs

TAL at the formatter errors in the CI

Copy link
Collaborator

@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: 3 of 10 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @Genya-Z)

a discussion (no related file):
please run ./scripts/rust_fmt.sh and `./scripts/cairo_fmt.sh



crates/cairo-lang-sierra/src/simulation/core.rs line 476 at r3 (raw file):

                        CoreValue::RangeCheck,
                        CoreValue::Uint64((x % 1<<64).try_into().unwrap()),
                        CoreValue::Uint64((x / 1<<64).try_into().unwrap()),

Suggestion:

                        CoreValue::RangeCheck,
                        CoreValue::Uint64((x & u64::MAX).try_into().unwrap()),
                        CoreValue::Uint64((x >> 64).try_into().unwrap()),

Copy link
Author

@Genya-Z Genya-Z 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: 3 of 10 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @orizi)

a discussion (no related file):

Previously, orizi wrote…

please run ./scripts/rust_fmt.sh and `./scripts/cairo_fmt.sh

I get

Code snippet:

error: no such command: `+nightly-2022-11-03`

        Cargo does not handle `+toolchain` directives.
        Did you mean to invoke `cargo` through `rustup` instead?


crates/cairo-lang-sierra/src/extensions/modules/uint128.rs line 28 at r6 (raw file):

Previously, dorimedini-starkware wrote…

TAL at the formatter errors in the CI

Done.


crates/cairo-lang-sierra/src/simulation/core.rs line 476 at r3 (raw file):

                        CoreValue::RangeCheck,
                        CoreValue::Uint64((x % 1<<64).try_into().unwrap()),
                        CoreValue::Uint64((x / 1<<64).try_into().unwrap()),

Done.

Copy link
Collaborator

@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: 1 of 10 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @Genya-Z)

a discussion (no related file):

Previously, Genya-Z wrote…

I get

if you'd run these it would conform your code to our formatting, and prevent the relevant ci failures.


Copy link
Collaborator

@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: 1 of 10 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @Genya-Z)


crates/cairo-lang-sierra-to-casm/src/invocations/uint128.rs line 218 at r2 (raw file):

Previously, dorimedini-starkware wrote…

@orizi WDYT?

it should be in the other order - to match u128s_from_felt - which is the existing split function.

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware 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 3 files at r1, 1 of 8 files at r6, 1 of 1 files at r8, 1 of 2 files at r9, 5 of 5 files at r11, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Genya-Z and @orizi)


corelib/src/integer.cairo line 766 at r11 (raw file):

const HALF_SHIFT : felt = 18446744073709551616;//2^64;

@Genya-Z any evidence that this is cheaper?
@orizi any suggestions on how to test if it is?

Copy link
Contributor

@dorimedini-starkware dorimedini-starkware 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: 3 of 10 files reviewed, 7 unresolved discussions (waiting on @Genya-Z and @orizi)

a discussion (no related file):

Previously, orizi wrote…

if you'd run these it would conform your code to our formatting, and prevent the relevant ci failures.

did you first run rustup override set stable && rustup update && cargo test as mentioned in the README?


Copy link
Contributor

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 5 files at r14, all commit messages.
Reviewable status: 6 of 10 files reviewed, 8 unresolved discussions (waiting on @Genya-Z and @orizi)


corelib/src/test.cairo line 694 at r14 (raw file):

    );

    let (low, high) = integer::u256_wide_mul(as_u256(4_u128, 3_u128), as_u256(0_u128, 1_u128));

please add this comnent

Suggestion:

// u256_wide_mul tests.
let (low, high) = integer::u256_wide_mul(as_u256(4_u128, 3_u128), as_u256(0_u128, 1_u128));

crates/cairo-lang-sierra/src/extensions/modules/uint128.rs line 197 at r14 (raw file):

                    ref_info: OutputVarReferenceInfo::NewTempVar { idx: Some(0) },
                },
                OutputVarInfo {

@orizi should these be with idx: 1, idx: 2 or 0,1 as it is now?

Code quote:

                OutputVarInfo {
                    ty: ty_u64.clone(),
                    ref_info: OutputVarReferenceInfo::NewTempVar { idx: Some(0) },
                },
                OutputVarInfo {
                    ty: ty_u64,
                    ref_info: OutputVarReferenceInfo::NewTempVar { idx: Some(1) },
                },

Copy link
Author

@Genya-Z Genya-Z 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: 6 of 10 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @orizi)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

did you first run rustup override set stable && rustup update && cargo test as mentioned in the README?

Done.



crates/cairo-lang-sierra-to-casm/src/invocations/uint128.rs line 218 at r2 (raw file):

Previously, orizi wrote…

it should be in the other order - to match u128s_from_felt - which is the existing split function.

Done.


docs/CODE_OF_CONDUCT.md line 1 at r2 (raw file):

Previously, dorimedini-starkware wrote…

why is this deleted?

Done.


corelib/test.cairo line at r6 (raw file):

Previously, dorimedini-starkware wrote…

why deleted?

File was moved.

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