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

Add u128_byte_reverse. #2839

Merged
merged 1 commit into from
May 7, 2023
Merged

Add u128_byte_reverse. #2839

merged 1 commit into from
May 7, 2023

Conversation

ilyalesokhin-starkware
Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware commented Apr 16, 2023

This change is Reviewable

@ilyalesokhin-starkware ilyalesokhin-starkware marked this pull request as draft April 16, 2023 15:33
@ilyalesokhin-starkware
Copy link
Contributor Author

tests/e2e_test_data/libfuncs/u128 line 730 at r1 (raw file):

[ap + 0] = [ap + -5] + [ap + -1], ap++;
[ap + 0] = 72057594037927936, ap++;
[ap + 0] = [ap + 0] * [ap + -1], ap++;

@orizi
This seems to be a bug.
it is supposed to be:
[ap + k] = [ap + (k-1)] * ...

for some k (maybe 0).

tempvar shifted_var = and * shift_temp;

Code quote:

[ap + 0] = [ap + 0] * [ap + -1], ap++;

@ilyalesokhin-starkware
Copy link
Contributor Author

tests/e2e_test_data/libfuncs/u128 line 730 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

@orizi
This seems to be a bug.
it is supposed to be:
[ap + k] = [ap + (k-1)] * ...

for some k (maybe 0).

tempvar shifted_var = and * shift_temp;

nvm, the issue is with:

tempvar temp = temp / shift_temp;

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: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)

a discussion (no related file):
Won't having split functions for u128 into u16 more efficient, and just widemul back into u64?


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: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)


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

    ];

    let mut temp = input;

won't this be just as efficient in user code?

Code quote:

    let masks = [
        0x00ff00ff00ff00ff00ff00ff00ff00ff_u128,
        0x00ffff0000ffff0000ffff0000ffff00_u128,
        0x00ffffffff00000000ffffffff000000_u128,
    ];

    let mut temp = input;
    let mut shift = BigInt::from(1 << 16);
    for mask_imm in masks.into_iter().map(BigInt::from) {
        let shift_imm = BigInt::from(&shift - 1);
        casm_build_extend! {casm_builder,
            assert temp = *(bitwise++);
            const mask_imm = mask_imm;
            const shift_imm = shift_imm;
            tempvar shift_temp = shift_imm;
            tempvar mask = mask_imm;
            assert mask = *(bitwise++);
            tempvar and = *(bitwise++);
            let _xor = *(bitwise++);
            let _or = *(bitwise++);

            tempvar shifted_var = and * shift_temp;
            tempvar x = temp + shifted_var;
        };

        shift = &shift * &shift;
        temp = x;
    }

@ilyalesokhin-starkware
Copy link
Contributor Author

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

Previously, orizi wrote…

won't this be just as efficient in user code?

What type would you use for temp?

A felt can't be assigned to bitwise safely, and u251 would add range checks.

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-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: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @orizi)

a discussion (no related file):

Previously, orizi wrote…

Won't having split functions for u128 into u16 more efficient, and just widemul back into u64?

I don't see how.


Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-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: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @orizi)

a discussion (no related file):

Previously, ilyalesokhin-starkware wrote…

I don't see how.

Can you elaborate?


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: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

a discussion (no related file):

Previously, ilyalesokhin-starkware wrote…

I don't see how.

First off - I don't remember how heavy is the bitwise builtin usage, so that would remove its need.
anyway - I think you should first implement whatever you wanted in user code, and then add libfuncs to improve the performance, not the other way around, easier to prove necessity, to design and to test.



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

Previously, ilyalesokhin-starkware wrote…

What type would you use for temp?

A felt can't be assigned to bitwise safely, and u251 would add range checks.

Temp is a legal u128, but it is indeed not checked at the time.
We still can logically split this to something which is simpler but useful.
This libfunc still seems to me to be doing something extremely specific

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: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

a discussion (no related file):

Previously, orizi wrote…

First off - I don't remember how heavy is the bitwise builtin usage, so that would remove its need.
anyway - I think you should first implement whatever you wanted in user code, and then add libfuncs to improve the performance, not the other way around, easier to prove necessity, to design and to test.

The main idea is just:
u128 into 16 u8, and recombination of the bytes.
but extra libfuncs would be required all around


Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-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: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @orizi)

a discussion (no related file):

Previously, orizi wrote…

The main idea is just:
u128 into 16 u8, and recombination of the bytes.
but extra libfuncs would be required all around

Implementing it in high level code is a lot more work.
Lior agreed that this should be a libfunc.



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

Previously, orizi wrote…

Temp is a legal u128, but it is indeed not checked at the time.
We still can logically split this to something which is simpler but useful.
This libfunc still seems to me to be doing something extremely specific

Temp is not a legal u128.
it is a valid
u{128+ (8 + 16 + 32)}

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: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)

a discussion (no related file):

Previously, ilyalesokhin-starkware wrote…

Implementing it in high level code is a lot more work.
Lior agreed that this should be a libfunc.

this is really little work:
fn foo(mut a: u128) -> (u64, u64) {
let mut v1: u64 = 0;
let b: u64 = a % 256; a /= 256; v1 *= b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let mut v2: u64 = 0;
let b: u64 = a % 256; a /= 256; v2 *= b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
(v1, v2)
}

of course it is a lot less efficient - but let get to the target before we start implementing stuff that we need to make actual decisions on.



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

            vec![
                OutputVarInfo {
                    ty: bitwise_ty,

this already exists - not sure why it appeared here.

Code quote:

/// Libfunc for computing the Bitwise (and,or,xor) of two u128s.
/// Returns 3 u128s (and the updated builtin pointer).
#[derive(Default)]
pub struct BitwiseLibfunc {}
impl NoGenericArgsGenericLibfunc for BitwiseLibfunc {
    const STR_ID: &'static str = "bitwise";

    fn specialize_signature(
        &self,
        context: &dyn SignatureSpecializationContext,
    ) -> Result<LibfuncSignature, SpecializationError> {
        let bitwise_ty = context.get_concrete_type(BitwiseType::id(), &[])?;
        let u128_ty = context.get_concrete_type(Uint128Type::id(), &[])?;
        Ok(LibfuncSignature::new_non_branch_ex(
            vec![
                ParamSignature {
                    ty: bitwise_ty.clone(),
                    allow_deferred: false,
                    allow_add_const: true,
                    allow_const: false,
                },
                ParamSignature::new(u128_ty.clone()),
                ParamSignature::new(u128_ty.clone()),
            ],
            vec![
                OutputVarInfo {
                    ty: bitwise_ty,
                    ref_info: OutputVarReferenceInfo::Deferred(DeferredOutputKind::AddConst {
                        param_idx: 0,
                    }),
                },
                OutputVarInfo {
                    ty: u128_ty.clone(),
                    ref_info: OutputVarReferenceInfo::Deferred(DeferredOutputKind::Generic),
                },
                OutputVarInfo {
                    ty: u128_ty.clone(),
                    ref_info: OutputVarReferenceInfo::Deferred(DeferredOutputKind::Generic),
                },
                OutputVarInfo {
                    ty: u128_ty,
                    ref_info: OutputVarReferenceInfo::Deferred(DeferredOutputKind::Generic),
                },
            ],
            SierraApChange::Known { new_vars_only: true },
        ))
    }
}

@ilyalesokhin-starkware
Copy link
Contributor Author

Previously, orizi wrote…

this is really little work:
fn foo(mut a: u128) -> (u64, u64) {
let mut v1: u64 = 0;
let b: u64 = a % 256; a /= 256; v1 *= b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let b: u64 = a % 256; a /= 256; v1 *= 256; v1 += b;
let mut v2: u64 = 0;
let b: u64 = a % 256; a /= 256; v2 *= b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
let b: u64 = a % 256; a /= 256; v2 *= 256; v2 += b;
(v1, v2)
}

of course it is a lot less efficient - but let get to the target before we start implementing stuff that we need to make actual decisions on.

it gives me
error: Unexpected argument type. Expected: "core::integer::u64", found: "core::integer::u128".
--> keccak.cairo:108:18
let b: u64 = a % 256;
^*****^
I might be able to make it work, but I think it is a waste of time.

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-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: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @orizi)


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

Previously, orizi wrote…

this already exists - not sure why it appeared here.

Done.

@ilyalesokhin-starkware ilyalesokhin-starkware marked this pull request as ready for review April 24, 2023 12:07
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: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)

a discussion (no related file):

Previously, ilyalesokhin-starkware wrote…

it gives me
error: Unexpected argument type. Expected: "core::integer::u64", found: "core::integer::u128".
--> keccak.cairo:108:18
let b: u64 = a % 256;
^*****^
I might be able to make it work, but I think it is a waste of time.

you'd just need to add .into()


@ilyalesokhin-starkware
Copy link
Contributor Author

Previously, orizi wrote…

you'd just need to add .into()

it didn't compile and I don't want to fight with it.

lior said we should add the libfunc.

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.

Reviewed 1 of 11 files at r5.
Reviewable status: 1 of 11 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)

a discussion (no related file):

Previously, ilyalesokhin-starkware wrote…

it didn't compile and I don't want to fight with it.

lior said we should add the libfunc.

this is not a good claim for why this paricular libfunc should exist instead of any of the other options.


@ilyalesokhin-starkware ilyalesokhin-starkware changed the title Add u128_to_reversed_u64s. Add u128_byte_reverse. May 7, 2023
Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-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: 0 of 14 files reviewed, 3 unresolved discussions (waiting on @orizi)

a discussion (no related file):

Previously, orizi wrote…

this is not a good claim for why this paricular libfunc should exist instead of any of the other options.

change to u128_byte_reverse.


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.

Reviewed 2 of 11 files at r5, 10 of 12 files at r6, all commit messages.
Reviewable status: 12 of 14 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)


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

#[test]
fn test_u128_byte_reverse() {
    let x = 0x000102030405060708090a0b0c0d0e0f_u128;

inline x and remove _u128


crates/cairo-lang-sierra/src/extensions/modules/int/unsigned128.rs line 309 at r6 (raw file):

        Ok(LibfuncSignature::new_non_branch_ex(
            vec![
                ParamSignature {

use the ParamSignature::new(..).with_.. pattern.


crates/cairo-lang-sierra-to-casm/src/invocations/int/unsigned128.rs line 451 at r6 (raw file):

            const mask_imm = mask_imm;
            const shift_imm = shift_imm;
            tempvar shift_temp = shift_imm;

no need for the extra tempvar - as this is a valid immediate for the shifting mul.

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.

:lgtm:

Reviewed 7 of 7 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-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: all files reviewed, 1 unresolved discussion (waiting on @orizi)


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

Previously, orizi wrote…

inline x and remove _u128

Done.


crates/cairo-lang-sierra/src/extensions/modules/int/unsigned128.rs line 309 at r6 (raw file):

Previously, orizi wrote…

use the ParamSignature::new(..).with_.. pattern.

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

@ilyalesokhin-starkware ilyalesokhin-starkware added this pull request to the merge queue May 7, 2023
Merged via the queue into main with commit 63194d7 May 7, 2023
@orizi orizi deleted the ilya/endian branch June 28, 2023 05:38
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