Registers in memory #1443
Merged
Registers in memory #1443
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leonardoalt
force-pushed
the
registers-in-memory-dev
branch
2 times, most recently
from
2284f46
to
2aedf98
Compare
leonardoalt
force-pushed
the
registers-in-memory-dev
branch
5 times, most recently
from
48195ed
to
8b67cba
Compare
leonardoalt
force-pushed
the
registers-in-memory-dev
branch
5 times, most recently
from
2a1b8f5
to
db4fcbb
Compare
leonardoalt
force-pushed
the
registers-in-memory-dev
branch
from
db4fcbb
to
5532422
Compare
This was referenced Jul 4, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 8, 2024
Implements #1055 for the Poseidon machines. Pulled out of #1508. Specifically, this PR adds a new `PoseidonGLMemory` machine which receives 2 memory points and then: - Reads 24 32-Bit words and packs them into 12 field elements - Computes the Poseidon permutation (just like `PoseidonGL`) - For each of the 4 output field elements, it: - Invokes the `SplitGL` machine to get the canonical `u64` representation - Writes the 8 32-Bit words to memory at the provided memory pointer The read and write memory regions can even overlap! 🎉 This should simplify our RISC-V machine, as the syscall already expects two memory pointers. We can simply pass it to the machine directly. I started doing that in #1533, but I think it makes sense to wait until #1443 is merged. To test: ``` cargo run -r pil test_data/std/poseidon_gl_test.asm -o output -f --export-csv --prove-with estark-starky ``` I recommend reviewing the diff between `std/machines/hash/poseidon_gl.asm` and `std/machines/hash/poseidon_gl_memory.asm` ### Discussion The overhead of the memory read / write is quite high (18 extra witness columns, see [this comment](https://github.com/powdr-labs/powdr/blob/40bdca4368c3accccb753aa35ac1027ccb8def0e/std/machines/hash/poseidon_gl_memory.asm#L13-L23), mostly because we now need to have the input available in all rows (which previously was only the case for the outputs). If we had offsets other than 0 and 1, this could be avoided. Doing 24 parallel memory reads in the first row would *not* help, because we'd have to add 24 witness columns (instead of 2 now) to store the result of the memory operation. A few more notes: - With Vadcop, 18 extra witness columns in a secondary machine is *a lot* better than introducing more registers (either "regular" registers or assignment registers) in the main machine - As mentioned [here](https://github.com/powdr-labs/powdr/blob/40bdca4368c3accccb753aa35ac1027ccb8def0e/std/machines/hash/poseidon_gl_memory.asm#L111-L113), we could get rid of two permutations if either: - We were able to express explicitly that we want to call at most one operation in the current row, or - We had an optimizer that would be smart enough to batch the memory reads and writes. - We could also have just 1 read or write at a time (instead of 2), but we'd have to increase the block size from 31 to 32 and the implementation would be more complicated. - We could also store the full final state of the Poseidon permutation, instead of just the first 4 elements. This would need 8 more witness columns to make the entire output available in all rows. Then, one could use the machine to implement a Poseidon sponge, instead of. - Looking at the bootloader, maybe it makes sense to pass 3 input pointers instead of 1: One for the first 4 elements, one for the next 4, and one for the capacity (often just a constant). For example, when computing a Merkle root, you'd pass pointers for the two children hashes and a pointer to the capacity constant.
…sters-in-memory-dev
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 17, 2024
This PR contains the following changes: - I renamed a bunch of functions `verify_*` to `run_pilcom_*`, because I think it better describes what they do - They all call `run_pilcom_with_backend_variant`, which works analogous to `gen_estark_proof_with_backend_variant` and `test_halo2_with_backend_variant` - In functions like `run_pilcom_test_file` (previously `verify_test_file`), which are used for many tests, we now test both the monolithic and composite backend variant (but share the generated witness & constants). This is anlogous to `gen_estark_proof` and `test_halo2` - In the RISC-V tests, we only test the composite variant, because with registers in memory (#1443) we don't expect the monolithic backend variant to work anymore.
georgwiese
reviewed
Jul 18, 2024
Comment on lines
+77
to
+83
| // TODO hacky way to find the degree of the main machine, fix. | ||
| let length = fixed_cols | ||
| .iter() | ||
| .find(|(col, _)| col == "main.STEP") | ||
| .unwrap() | ||
| .1 | ||
| .len() as u64; |
There was a problem hiding this comment.
I think the correct way would be to include the length in the bootloader inputs.
lvella
reviewed
Jul 22, 2024
| data_code.push(format!("mstore 0x{addr:x}, 0x{v:x};")); | ||
| data_code.push(format!( | ||
| "set_reg {}, 0x{v:x};", | ||
| Register::from("tmp2").addr() |
There was a problem hiding this comment.
May create a plain function fn reg_addr(&str) -> u32 ?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1492