Apply comments

0xPolygonZero · Nov 9, 2023 · 9ef6cf6 · 9ef6cf6
1 parent 515eead
commit 9ef6cf6
Show file tree

Hide file tree

Showing 11 changed files with 32 additions and 31 deletions.
diff --git a/evm/src/cpu/columns/mod.rs b/evm/src/cpu/columns/mod.rs
@@ -94,8 +94,10 @@ pub struct CpuColumnsView<T: Copy> {
     /// CPU clock.
     pub(crate) clock: T,
 
-    /// Memory bus channels in the CPU. Each channel is comprised of 13 columns.
+    /// Memory bus channels in the CPU.
+    /// Full channels are comprised of 13 columns.
     pub mem_channels: [MemoryChannelView<T>; NUM_GP_CHANNELS],
+    /// Partial channel is only comprised of 5 columns.
     pub partial_channel: PartialMemoryChannelView<T>,
 }
 

diff --git a/evm/src/cpu/cpu_stark.rs b/evm/src/cpu/cpu_stark.rs
@@ -34,7 +34,7 @@ pub fn ctl_data_keccak_sponge<F: Field>() -> Vec<Column<F>> {
     // GP channel 1: stack[-2] = segment
     // GP channel 2: stack[-3] = virt
     // GP channel 3: stack[-4] = len
-    // GP channel 4: pushed = outputs
+    // Next GP channel 0: pushed = outputs
     let context = Column::single(COL_MAP.mem_channels[0].value[0]);
     let segment = Column::single(COL_MAP.mem_channels[1].value[0]);
     let virt = Column::single(COL_MAP.mem_channels[2].value[0]);
@@ -127,7 +127,7 @@ pub fn ctl_data_byte_unpacking<F: Field>() -> Vec<Column<F>> {
     // GP channel 1: stack[-2] = segment
     // GP channel 2: stack[-3] = virt
     // GP channel 3: stack[-4] = val
-    // GP channel 4: pushed = new_offset (virt + len)
+    // Next GP channel 0: pushed = new_offset (virt + len)
     let context = Column::single(COL_MAP.mem_channels[0].value[0]);
     let segment = Column::single(COL_MAP.mem_channels[1].value[0]);
     let virt = Column::single(COL_MAP.mem_channels[2].value[0]);

diff --git a/evm/src/cpu/kernel/asm/core/syscall.asm b/evm/src/cpu/kernel/asm/core/syscall.asm
@@ -117,8 +117,13 @@ global syscall_jumptable:
     JUMPTABLE sys_log2
     JUMPTABLE sys_log3
     JUMPTABLE sys_log4
-    %rep 27
-        JUMPTABLE panic // 0xa5-0xbf are invalid opcodes
+    %rep 11
+        JUMPTABLE panic // 0xa5-0xaf are invalid opcodes
+    %endrep
+
+    // 0xb0-0xbf
+    %rep 16
+        JUMPTABLE panic // 0xb0-0xbf are invalid opcodes
     %endrep
 
     // 0xc0-0xdf

diff --git a/evm/src/cpu/kernel/asm/memory/memset.asm b/evm/src/cpu/kernel/asm/memory/memset.asm
@@ -29,7 +29,7 @@ global memset:
     MSTORE_32BYTES_32
     // stack: new_offset, DST, count, retdest
 
-    // Increment dst_addr.
+    // Update dst_addr.
     SWAP3
     POP
     // Decrement count.

diff --git a/evm/src/cpu/kernel/asm/memory/packing.asm b/evm/src/cpu/kernel/asm/memory/packing.asm
@@ -41,20 +41,29 @@ global mload_packing_u64_LE:
 // Pre stack: context, segment, offset, value, len, retdest
 // Post stack: offset'
 global mstore_unpacking:
+    // stack: context, segment, offset, value, len, retdest
+    DUP5 ISZERO
+    // stack: len == 0, context, segment, offset, value, len, retdest
+    %jumpi(mstore_unpacking_empty)
     %stack(context, segment, offset, value, len, retdest) -> (len, context, segment, offset, value, retdest)
     PUSH 3
-    //stack: BYTES_PER_JUMP, len, context, segment, offset, value, retdest
+    // stack: BYTES_PER_JUMP, len, context, segment, offset, value, retdest
     MUL
-    //stack: jump_offset, context, segment, offset, value, retdest
+    // stack: jump_offset, context, segment, offset, value, retdest
     PUSH mstore_unpacking_0
-    //stack: mstore_unpacking_0, jump_offset, context, segment, offset, value, retdest
+    // stack: mstore_unpacking_0, jump_offset, context, segment, offset, value, retdest
     ADD
-    //stack: address_unpacking, context, segment, offset, value, retdest
+    // stack: address_unpacking, context, segment, offset, value, retdest
     JUMP
 
+mstore_unpacking_empty:
+    %stack(context, segment, offset, value, len, retdest) -> (retdest, offset)
+    JUMP
+
+// This case can never be reached. It's only here to offset the table correctly.
 mstore_unpacking_0:
     %rep 3
-    PUSH 0
+        PANIC
     %endrep
 mstore_unpacking_1:
     // stack: context, segment, offset, value, retdest

diff --git a/evm/src/cpu/kernel/asm/memory/syscalls.asm b/evm/src/cpu/kernel/asm/memory/syscalls.asm
@@ -29,7 +29,7 @@ global sys_mstore:
     %stack(kexit_info, offset, value) -> (offset, value, kexit_info)
     PUSH @SEGMENT_MAIN_MEMORY
     GET_CONTEXT
-    // stack: addr: 3, value, len, kexit_info
+    // stack: addr: 3, value, kexit_info
     MSTORE_32BYTES_32
     // stack: kexit_info
     EXIT_KERNEL

diff --git a/evm/src/cpu/kernel/opcodes.rs b/evm/src/cpu/kernel/opcodes.rs
@@ -4,7 +4,7 @@ pub fn get_push_opcode(n: u8) -> u8 {
     0x5f + n
 }
 
-/// The opcode of a standard instruction (not a `PUSH` or a `MSTORE_32BYTES`).
+/// The opcode of a standard instruction (not a `PUSH`).
 pub fn get_opcode(mnemonic: &str) -> u8 {
     match mnemonic.to_uppercase().as_str() {
         "STOP" => 0x00,

diff --git a/evm/src/cpu/membus.rs b/evm/src/cpu/membus.rs
@@ -49,7 +49,7 @@ pub fn eval_packed<P: PackedField>(
         yield_constr.constraint(channel.used * (channel.used - P::ONES));
     }
 
-    // Validate partial channel used flags. They should be binary.
+    // Validate `partial_channel.used`. It should be binary.
     yield_constr.constraint(lv.partial_channel.used * (lv.partial_channel.used - P::ONES));
 }
 
@@ -74,7 +74,7 @@ pub fn eval_ext_circuit<F: RichField + Extendable<D>, const D: usize>(
         yield_constr.constraint(builder, constr);
     }
 
-    // Validate partial channel used flags. They should be binary.
+    // Validate `partial_channel.used`. It should be binary.
     {
         let constr = builder.mul_sub_extension(
             lv.partial_channel.used,

diff --git a/evm/src/generation/mod.rs b/evm/src/generation/mod.rs
@@ -290,7 +290,6 @@ fn simulate_cpu<F: RichField + Extendable<D>, const D: usize>(
     state: &mut GenerationState<F>,
 ) -> anyhow::Result<()> {
     let halt_pc = KERNEL.global_labels["halt"];
-    let bootstrapping_len = state.traces.clock();
 
     loop {
         // If we've reached the kernel's halt routine, and our trace length is a power of 2, stop.
@@ -319,12 +318,6 @@ fn simulate_cpu<F: RichField + Extendable<D>, const D: usize>(
                 }
             }
 
-            // First CPU row's memory channel 0 must contain the kernel hash to make
-            // the Keccak CTL check pass.
-            let first_cpu_row = &mut state.traces.cpu[bootstrapping_len];
-            first_cpu_row.mem_channels[0].value = KERNEL.code_hash.map(F::from_canonical_u32);
-            first_cpu_row.mem_channels[0].value.reverse();
-
             log::info!("CPU trace padded to {} cycles", state.traces.clock());
 
             return Ok(());

diff --git a/evm/src/witness/operation.rs b/evm/src/witness/operation.rs
@@ -862,13 +862,6 @@ pub(crate) fn generate_mstore_general<F: Field>(
             .map_err(|_| MemoryError(VirtTooLarge { virt }))?,
     };
     let log_write = mem_write_partial_log_and_fill(address, state, &mut row, val);
-    // Write in partial channel.
-    let channel = &mut row.partial_channel;
-    channel.used = F::ONE;
-    channel.is_read = F::ZERO;
-    channel.addr_context = F::from_canonical_usize(address.context);
-    channel.addr_segment = F::from_canonical_usize(address.segment);
-    channel.addr_virtual = F::from_canonical_usize(address.virt);
 
     let diff = row.stack_len - F::from_canonical_usize(4);
     if let Some(inv) = diff.try_inverse() {

diff --git a/evm/src/witness/util.rs b/evm/src/witness/util.rs
@@ -205,8 +205,7 @@ pub(crate) fn mem_write_partial_log_and_fill<F: Field>(
 
     let channel = &mut row.partial_channel;
     assert!(channel.used.is_zero());
-    // channel.used = F::ONE;
-    // Flags set elsewhere.
+    channel.used = F::ONE;
     channel.is_read = F::ZERO;
     channel.addr_context = F::from_canonical_usize(address.context);
     channel.addr_segment = F::from_canonical_usize(address.segment);