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

Use longer integers to fill ZKASM data section #187

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Conversation

aborg-dev
Copy link

Because the constants are 64-bit values, we need to use a different integer type in the assignment.

Because the constants are 64-bit values, we need to use a different integer type in assignment.
@aborg-dev aborg-dev requested review from mooori and MCJOHN974 January 9, 2024 08:00
Comment on lines -14 to -22
SP + 5 => SP
SP + 4 => SP
C :MSTORE(SP - 1)
D :MSTORE(SP - 2)
E :MSTORE(SP - 3)
B :MSTORE(SP - 4)
1n => A ;; LoadConst64
0n => B ;; LoadConst64
10000n => C ;; LoadConst32
C => D
Copy link

Choose a reason for hiding this comment

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

Why does register allocation become smarter if this zkasm file has no data segment? Naively thinking, shouldn't this zkasm remain unchanged as the code path that was modified isn't hit? For fibonacci/from_rust the length of data_segments is zero in generate_preamble(..., data_segments), so the loop that emits {chunk_data}n ... isn't entered.

Copy link
Author

Choose a reason for hiding this comment

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

I also find this surprising and have seen this non-stable behavior by the codegen quite a few times. I've filed an issue to investigate it in more detail #190.

Unfortunately, we can't submit the change in this PR without the diff to the generated zkasm files as our tests require the snapshots to match. We should think how to handle this case in the new testing infrastructure. CC @MCJOHN974

@aborg-dev aborg-dev added this pull request to the merge queue Jan 9, 2024
Merged via the queue into main with commit cefafb6 Jan 9, 2024
25 checks passed
@aborg-dev aborg-dev deleted the global_const branch January 9, 2024 12:39
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