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

Store top of the stack in memory channel 0 #1215

Merged
merged 17 commits into from
Oct 11, 2023

Conversation

hratoanina
Copy link
Contributor

@hratoanina hratoanina commented Sep 6, 2023

Updated version of #1196. This one only introduces one extra column (by reintroducing the get_context and set_context flag).

This PR stores the current top of the stack in memory channel 0, saving memory operations in a lot of cases.
Reading and writing the new top of the stack after an operation is straightforward, but the most annoying thing was a disjunction depending on whether the current stack was empty or not for pushing instructions (resp. the next stack was empty or not for popping instructions), and whether or not to disable a channel in that case. This was done with prover-provided inverses which could be stored in general CPU columns, thus not adding any new columns.

Further work:
I couldn't get to mix this PR with the column merging while keeping stack constraint degrees below 3. I have an inkling this should be possible with more general columns though.

closes #1149

@hratoanina
Copy link
Contributor Author

Some figures about the memory trace reduction:

add11
517719 -> 440537 --> 14.9% reduction

(on a slightly outdated branch but still relevant)
pairingTest_0
6916972 -> 5498226 --> 20.5% reduction
ecmul_0-0_0_21000_0
1654468 -> 1443551 --> 12.7% reduction
ecadd_0-0_0-0_21000_0_0
1654939 -> 1443587 --> 12.8% reduction
CallSha256_0
883714 -> 769612 --> 12.9% reduction
CALLCODEBlake2f_0
509411 -> 439444 --> 13.7% reduction

@hratoanina
Copy link
Contributor Author

Failures are from interpreter tests, since I forgot to update the top_stack logic there, but other integration tests pass. I'm working on fixing this.

@hratoanina
Copy link
Contributor Author

@wborgeaud
It's a big one, sorry about that! Don't hesitate to mention anything about the design choices, there were a lot of arbitrary ones.

Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

Not an actual review, just quickly skimmed through. Only the comment on decode.rs matters though, the rest is nitpicking cleanup.

evm/src/cpu/decode.rs Outdated Show resolved Hide resolved
evm/src/cpu/jumps.rs Outdated Show resolved Hide resolved
evm/src/cpu/jumps.rs Outdated Show resolved Hide resolved
evm/src/cpu/kernel/interpreter.rs Outdated Show resolved Hide resolved
@Nashtare Nashtare added this to the Optimization - Phase 1 milestone Sep 11, 2023
@nbgl nbgl self-requested a review September 15, 2023 19:03
Copy link
Contributor

@nbgl nbgl left a comment

Choose a reason for hiding this comment

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

This is excellent work!! Thank you.

I'm sorry it took me so long to review this.

evm/src/cpu/columns/general.rs Outdated Show resolved Hide resolved
evm/src/witness/state.rs Show resolved Hide resolved
evm/src/witness/memory.rs Show resolved Hide resolved
evm/src/cpu/shift.rs Outdated Show resolved Hide resolved
evm/src/cpu/membus.rs Outdated Show resolved Hide resolved
evm/src/cpu/dup_swap.rs Outdated Show resolved Hide resolved
@hratoanina
Copy link
Contributor Author

Thanks for the review! We're not merging right now since we found some tests from the EVM test suite which aren't passing.

@hratoanina hratoanina merged commit 1d60431 into 0xPolygonZero:main Oct 11, 2023
2 checks passed
@Nashtare Nashtare deleted the top_stack_no_cols branch November 3, 2023 21:59
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.

Store the top of the stack in the CPU table
4 participants