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

feat(MemcpyEnterScope): enters a new scope for the memory copy operaton with a specified length #372

Merged
merged 7 commits into from
May 1, 2024

Conversation

Alvarodb
Copy link
Contributor

@Alvarodb Alvarodb commented Apr 25, 2024

resolves #250

@MaksymMalicki
Copy link
Contributor

Thank you for you contribution! If you resolve the the len as Felt and load it to the scope, you can try and write some unit tests, which check the insides of the scope. Write on Telegram if you have any problems 😁

@Alvarodb
Copy link
Contributor Author

Thank you for you contribution! If you resolve the the len as Felt and load it to the scope, you can try and write some unit tests, which check the insides of the scope. Write on Telegram if you have any problems 😁

Thank you for all the help! I will work on the unit tests.

@Alvarodb
Copy link
Contributor Author

Alvarodb commented Apr 26, 2024

unit test added for the hint :)

@MaksymMalicki
Copy link
Contributor

One nitpick left, other than that, looks good to me. Good job! 😎
@cicr99 final look?

@MaksymMalicki
Copy link
Contributor

Okey, you need to lint is as well using gofmt -w . or golangci-lint run

@Alvarodb
Copy link
Contributor Author

Okey, you need to lint is as well using gofmt -w . or golangci-lint run

Oh sure, fixed

@Alvarodb
Copy link
Contributor Author

Alvarodb commented Apr 27, 2024

One nitpick left, other than that, looks good to me. Good job! 😎 @cicr99 final look?

I think everything is fixed now guys, thank you very much! Let me know if any issues.

pkg/hintrunner/zero/zerohint_memcpy.go Outdated Show resolved Hide resolved
pkg/hintrunner/zero/zerohint_memcpy.go Show resolved Hide resolved
@TAdev0 TAdev0 marked this pull request as ready for review April 27, 2024 20:42
@TAdev0
Copy link
Member

TAdev0 commented Apr 28, 2024

LGTM ! waiting for final review @rodrigo-pino @chee-chyuan @MaksymMalicki @har777 and we're good to go :)

@TAdev0
Copy link
Member

TAdev0 commented Apr 29, 2024

@Alvarodb will compare the result of your hint with the result produced by the python VM today to be 100% sure and then we're good !

@Alvarodb
Copy link
Contributor Author

@Alvarodb will compare the result of your hint with the result produced by the python VM today to be 100% sure and then we're good !

Ok, sounds good! Thank you @TAdev0

@TAdev0
Copy link
Member

TAdev0 commented Apr 29, 2024

@Alvarodb i just tested your hint. Well, there is not much to test actually. The len variable is simply extracted and passed when entering a new scope, so that the new scope contains len as the only key, before copying memory from a location to another.

I used this cairo file that i ran on the python VM :

from starkware.cairo.common.alloc import alloc
from starkware.cairo.common.math import assert_not_zero

// Copies len field elements from src to dst.
func memcpy(dst: felt*, src: felt*, len) {
    struct LoopFrame {
        dst: felt*,
        src: felt*,
    }

    if (len == 0) {
        return ();
    }

    %{ vm_enter_scope({'n': ids.len}) %}
    %{ print(n) %}

    tempvar frame = LoopFrame(dst=dst, src=src);

    loop:
    let frame = [cast(ap - LoopFrame.SIZE, LoopFrame*)];
    assert [frame.dst] = [frame.src];

    let continue_copying = [ap];
    // Reserve space for continue_copying.
    let next_frame = cast(ap + 1, LoopFrame*);
    next_frame.dst = frame.dst + 1, ap++;
    next_frame.src = frame.src + 1, ap++;
    %{
        n -= 1
        ids.continue_copying = 1 if n > 0 else 0
    %}
    static_assert next_frame + LoopFrame.SIZE == ap + 1;
    jmp loop if continue_copying != 0, ap++;
    // Assert that the loop executed len times.
    len = cast(next_frame.src, felt) - cast(src, felt);

    %{ vm_exit_scope() %}
    return ();
}

func main() {
    alloc_locals;

    let (dest: felt*) = alloc();
    let source: felt* = alloc();

    assert source[0] = 1;
    assert source[1] = 1;
    assert source[2] = 1;

    memcpy(dest, source, 3);

    assert_not_zero(dest[0]);
    assert dest[0] = source[0];
    assert_not_zero(dest[1]);
    assert dest[1] = source[1];
    assert_not_zero(dest[2]);
    assert dest[2] = source[2];

    return ();
}

I added a new hint %{ print(n) %} to print the value of n in the scope. Then, in main, i call memcpy(dest, source, 3); to test here with len = 3 .

When i run this main function, i get 3 as output
Then, in the test file you created, i used 3 as an expected value for n when defining len as 3.

I also tested for fun with many other values, small or big, everything works well without surprise.
I guess testing this hint with the python cairo vm was not necessary but now we're 100% sure :)

@TAdev0
Copy link
Member

TAdev0 commented Apr 29, 2024

@rodrigo-pino @cicr99 @MaksymMalicki @har777 ready for a 2nd approval !

cicr99
cicr99 previously requested changes Apr 30, 2024
Copy link
Contributor

@cicr99 cicr99 left a comment

Choose a reason for hiding this comment

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

It looks great in general but there is the problem I pointed out in the comment. I see you wrote on top of the VMEnterScope hint, they are very similar but they are different ones, so you'll need to add memcpyEnterScope as a separate one.
See here an example where VMEnterScope is being used https://github.com/starkware-libs/cairo-lang/blob/efa9648f57568aad8f8a13fbf027d2de7c63c2c0/src/starkware/cairo/common/squash_dict.cairo#L28

pkg/hintrunner/zero/hintcode.go Show resolved Hide resolved
@Alvarodb
Copy link
Contributor Author

Alvarodb commented Apr 30, 2024

It looks great in general but there is the problem I pointed out in the comment. I see you wrote on top of the VMEnterScope hint, they are very similar but they are different ones, so you'll need to add memcpyEnterScope as a separate one. See here an example where VMEnterScope is being used https://github.com/starkware-libs/cairo-lang/blob/efa9648f57568aad8f8a13fbf027d2de7c63c2c0/src/starkware/cairo/common/squash_dict.cairo#L28

I've added memcpyEnterScope as a separate function, keeping the original implementation of VMEnterScope intact so this should be ok now. Thanks for the feedback!

@Alvarodb Alvarodb requested a review from cicr99 April 30, 2024 15:35
pkg/hintrunner/zero/hintcode.go Outdated Show resolved Hide resolved
@Alvarodb Alvarodb requested a review from TAdev0 April 30, 2024 17:37
@TAdev0
Copy link
Member

TAdev0 commented Apr 30, 2024

LGTM @cicr99

@har777
Copy link
Contributor

har777 commented May 1, 2024

lgtm!

@TAdev0 TAdev0 dismissed cicr99’s stale review May 1, 2024 12:04

2 approvals already, ready to merge

@TAdev0 TAdev0 merged commit 44af84b into NethermindEth:main May 1, 2024
4 checks passed
@Alvarodb Alvarodb deleted the implement-MemcpyEnterScope branch May 1, 2024 14:57
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.

MemcpyEnterScope
5 participants