[feat] modular add v2 #510

luffykai · 2024-10-08T21:06:08Z

part of INT-2260

add new heap_adapter, currently assumes it's always 2 read 1 write
make a new version of modular add chip with the new adapter, and the field expression framework. Currently only runtime, no tracegen or eval

moved some utils functions so touched many files, but only modular_v2 and rv32_heap.rs are relevant

TODO for follow up PRs

make number of read/write generic? like can the same adapter handles 1read1write and 2read1write? Is there other patterns?
trace gen and eval of the adapter , and the new chip in general
Add select to field expression framework to support add or sub in the same chip

linear · 2024-10-08T21:06:40Z

INT-2260 Runtime for modular arithmetic and elliptic curve operations

Relies on being able to figure out moduli handling.

vm/src/modular_v2/tests.rs

vm/src/arch/adapters/rv32_heap.rs

vm/src/modular_v2/mod.rs

jonathanpwang · 2024-10-08T21:46:47Z

vm/src/modular_v2/mod.rs

+        let x_biguint = limbs_to_biguint(&x, LIMB_SIZE);
+        let y_biguint = limbs_to_biguint(&y, LIMB_SIZE);
+
+        let z_biguint = (x_biguint + y_biguint) % &self.modulus;


are you doing it this way for convenience for now? shouldn't this be owned by ExprBuilder? also you don't handle Sub

ExprBuilder currently doesn't do the "execute" part explicitly. It kinda does it as part of trace gen, but need some work to get it done more cleanly.

doing sub here requires ExprBuilder to support select

Since this PR was intended for register-heap adapter, going to add the above in a followup PR

OK this execute thing about ExprBuilder will need to be resolved right, as that is how we will plan to use it throughout.

jonathanpwang

Adapter: shouldn't be using old MemoryHeap stuff because the pointer needs to come from RV32 register, not single field element cell.

Integration: only handles Add and not Sub (maybe that was a todo), but also doesn't use ExprBuilder so I'm confused.

jonathanpwang · 2024-10-10T15:35:45Z

vm/src/arch/adapters/rv32_heap.rs

+pub struct Rv32HeapAdapterCols<T, const READ_SIZE: usize, const WRITE_SIZE: usize> {
+    pub read_aux: [Rv32RegisterHeapReadAuxCols<T, READ_SIZE>; 2],
+    pub write_aux: Rv32RegisterHeapWriteAuxCols<T, WRITE_SIZE>,
+    pub read_addresses: [HeapAddress<T, T>; 2],


This should be addressed when you finish the AIR, but I don't think HeapAddress is optimal: the address space for register is fixed to 1 so it shouldn't be a column. The address space for memory is the same address read/writes so it should be at most a single column for e. Also at present for intrinsics I decided we can just fix that address space to 2. So HeapAddress can save some columns.

jonathanpwang · 2024-10-10T15:36:35Z

vm/src/arch/adapters/rv32_heap.rs

+
+#[repr(C)]
+#[derive(Clone, Debug, AlignedBorrow)]
+pub struct Rv32RegisterHeapReadAuxCols<T, const N: usize> {


In general, adapter specific columns and methods should be put in the adapter file and not in memory/ since that should be kept for core memory functionality. I have moved everything for registerheap out. The old MemoryHeap__ structs and methods should eventually be moved out too.

Additionally, since we aren't separating by module, any naming involving register should specify RV32 register (since RV64 would be different if we ever chose to support it).

* modular add v2 * use register * dont put range checker bits as config * add todo * fix test * fix test * refactor: move structs out of memory into adapter mod * add todo * fix --------- Co-authored-by: Jonathan Wang <[email protected]>

luffykai commented Oct 8, 2024

View reviewed changes

vm/src/modular_v2/tests.rs Show resolved Hide resolved

vm/src/arch/adapters/rv32_heap.rs Show resolved Hide resolved

luffykai marked this pull request as ready for review October 8, 2024 21:17

luffykai requested a review from jonathanpwang October 8, 2024 21:19