Chore/Redesign VM trace generation #109
Conversation
| macro_rules! read { | ||
| ($address_space: expr, $address: expr) => {{ | ||
| num_reads += 1; | ||
| assert!(num_reads <= MAX_READS_PER_CYCLE); | ||
| let timestamp = (MAX_ACCESSES_PER_CYCLE * clock_cycle) + (num_reads - 1); | ||
| let data = if $address_space == F::zero() { | ||
| decompose::<WORD_SIZE, F>($address) | ||
| } else { | ||
| vm.memory_chip | ||
| .read_word(timestamp, $address_space, $address) | ||
| }; | ||
| accesses[num_reads - 1] = | ||
| memory_access_to_cols(true, $address_space, $address, data); | ||
| compose(data) | ||
| }}; | ||
| } | ||
|
|
||
| macro_rules! write { | ||
| ($address_space: expr, $address: expr, $data: expr) => {{ | ||
| num_writes += 1; | ||
| assert!(num_writes <= MAX_WRITES_PER_CYCLE); | ||
| let timestamp = (MAX_ACCESSES_PER_CYCLE * clock_cycle) | ||
| + (MAX_READS_PER_CYCLE + num_writes - 1); | ||
| let word = decompose($data); | ||
| vm.memory_chip | ||
| .write_word(timestamp, $address_space, $address, word); | ||
| accesses[MAX_READS_PER_CYCLE + num_writes - 1] = | ||
| memory_access_to_cols(true, $address_space, $address, word); | ||
| }}; | ||
| } | ||
|
|
There was a problem hiding this comment.
I should point this out explicitly: I initially hoped to make functions/closures for read/write in CPU trace generation to reduce code duplication, but I ran into issues with multiple borrows. I currently have it written using macros which I'm assuming is not ideal. This could be changed to using closures where we would simply have to pass in vm and accesses, which makes the following code less nice but may be preferable to macros.
TlatoaniHJ
force-pushed
the
chore/vm-trace
branch
from
578e2d7 to
0d7d797
Compare
| let program = builder.compile_isa(); | ||
| execute_program::<WORD_SIZE, _>(program); | ||
| // let program = builder.compile_isa(); | ||
| // execute_program::<WORD_SIZE, _>(program); |
There was a problem hiding this comment.
test_compiler_break assumes that arrays are initialized with 0, meaning that it currently fails as we do not currently take memory to be preinitialized with 0. We should probably decide whether we want to guarantee that arrays are initialized with 0 or not (I would guess that we don't want to for performance reasons) but if we do, this is probably a change that should be made in the future, so for now I commented out the test.
| vm: VmParamsConfig { | ||
| field_arithmetic_enabled: true, | ||
| limb_bits: 28, | ||
| decomp: 4, |
There was a problem hiding this comment.
decomp can actually be much larger, but are we requiring decomp divides limb_bits? Just making a note that this assumption can be removed with an update to LessThanAir.
There was a problem hiding this comment.
I chose decomp arbitrarily for now, since I assume that we will want it to be configurable eventually.
| pub memory_air: OfflineChecker, | ||
| pub field_arithmetic_air: FieldArithmeticAir, | ||
| pub program_chip: ProgramChip<F>, | ||
| pub memory_chip: MemoryChip<WORD_SIZE, F>, |
There was a problem hiding this comment.
I feel like somewhere either in VirtualMachine or the chip internals, we'll soon need some smart pointers like Arc<Mutex to get around mutable borrow issues, but we can cross that road when we get to it.
There was a problem hiding this comment.
Already ran into that with the CPU (see my comment), but probably the current solution there is good enough that it's not worth the effort for now. Maybe will look into it when I have unused time
* Refactor memory chip and program chip * Progress (currently debugging CPU tests) * All tests passing * Remove warning for read/write macro in cpu trace * Add in change that got left out * Lint field_arithmetic/trace.rs * Fix lint, remove rust backtrace line * Fix compiler/src/util.rs, remove debugging code * Comment out test_compiler_break
Refactor VM trace generation to the new design explained in Aggregation VM STARK Spec under New Trace Generation. This involves refactoring all existing chips (CPU, program, memory, field arithmetic). Authors of these chips should read through the refactored tests if possible to confirm that their intent is preserved.