Skip to content

Commit

Permalink
fix: fix memory tests
Browse files Browse the repository at this point in the history
  • Loading branch information
plafer committed Dec 11, 2024
1 parent fa9d6c1 commit 6eadbd9
Show file tree
Hide file tree
Showing 17 changed files with 529 additions and 309 deletions.
4 changes: 2 additions & 2 deletions air/src/trace/chiplets/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ pub const ELEMENT_OR_WORD_COL_IDX: usize = READ_WRITE_COL_IDX + 1;
/// Column to hold the context ID of the current memory context.
pub const CTX_COL_IDX: usize = ELEMENT_OR_WORD_COL_IDX + 1;
/// Column to hold the memory address.
pub const ADDR_COL_IDX: usize = CTX_COL_IDX + 1;
pub const BATCH_COL_IDX: usize = CTX_COL_IDX + 1;
/// Column to hold the first bit of the index of the address in the batch.
pub const IDX0_COL_IDX: usize = ADDR_COL_IDX + 1;
pub const IDX0_COL_IDX: usize = BATCH_COL_IDX + 1;
/// Column to hold the second bit of the index of the address in the batch.
pub const IDX1_COL_IDX: usize = IDX0_COL_IDX + 1;
/// Column for the clock cycle in which the memory operation occurred.
Expand Down
2 changes: 1 addition & 1 deletion air/src/trace/chiplets/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ pub const MEMORY_SELECTORS_COL_IDX: usize = MEMORY_TRACE_OFFSET;
/// The index within the main trace of the column containing the memory context.
pub const MEMORY_CTX_COL_IDX: usize = MEMORY_TRACE_OFFSET + memory::CTX_COL_IDX;
/// The index within the main trace of the column containing the memory address.
pub const MEMORY_ADDR_COL_IDX: usize = MEMORY_TRACE_OFFSET + memory::ADDR_COL_IDX;
pub const MEMORY_ADDR_COL_IDX: usize = MEMORY_TRACE_OFFSET + memory::BATCH_COL_IDX;
/// The index within the main trace of the column containing the clock cycle of the memory
/// access.
pub const MEMORY_CLK_COL_IDX: usize = MEMORY_TRACE_OFFSET + memory::CLK_COL_IDX;
Expand Down
9 changes: 4 additions & 5 deletions miden/src/cli/debug/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ impl DebugExecutor {

/// print all memory entries.
pub fn print_memory(&self) {
for (address, mem) in self.vm_state.memory.iter() {
for &(address, mem) in self.vm_state.memory.iter() {
Self::print_memory_data(address, mem)
}
}
Expand All @@ -167,7 +167,7 @@ impl DebugExecutor {
});

match entry {
Some(mem) => Self::print_memory_data(&address, mem),
Some(&mem) => Self::print_memory_data(address, mem),
None => println!("memory at address '{address}' not found"),
}
}
Expand All @@ -176,9 +176,8 @@ impl DebugExecutor {
// --------------------------------------------------------------------------------------------

/// print memory data.
fn print_memory_data(address: &u64, memory: &[Felt]) {
let mem_int = memory.iter().map(|&x| x.as_int()).collect::<Vec<_>>();
println!("{address} {mem_int:?}");
fn print_memory_data(address: u64, mem_value: Felt) {
println!("{address} {mem_value:?}");
}

/// print help message
Expand Down
19 changes: 9 additions & 10 deletions miden/src/repl/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{collections::BTreeSet, path::PathBuf};

use assembly::{Assembler, Library};
use miden_vm::{math::Felt, DefaultHost, StackInputs, Word};
use miden_vm::{math::Felt, DefaultHost, StackInputs};
use processor::ContextId;
use rustyline::{error::ReadlineError, DefaultEditor};
use stdlib::StdLibrary;
Expand Down Expand Up @@ -171,7 +171,7 @@ pub fn start_repl(library_paths: &Vec<PathBuf>, use_stdlib: bool) {
let mut should_print_stack = false;

// state of the entire memory at the latest clock cycle.
let mut memory: Vec<(u64, Word)> = Vec::new();
let mut memory: Vec<(u64, Felt)> = Vec::new();

// initializing readline.
let mut rl = DefaultEditor::new().expect("Readline couldn't be initialized");
Expand Down Expand Up @@ -224,9 +224,9 @@ pub fn start_repl(library_paths: &Vec<PathBuf>, use_stdlib: bool) {
println!("The memory has not been initialized yet");
continue;
}
for (addr, mem) in &memory {
for &(addr, mem) in &memory {
// prints out the address and memory value at that address.
print_mem_address(*addr, mem);
print_mem_address(addr, mem);
}
} else if line.len() > 6 && &line[..5] == "!mem[" {
// if user wants to see the state of a particular address in a memory, the input
Expand All @@ -238,8 +238,8 @@ pub fn start_repl(library_paths: &Vec<PathBuf>, use_stdlib: bool) {
// extracts the address from user input.
match read_mem_address(&line) {
Ok(addr) => {
for (i, memory_value) in &memory {
if *i == addr {
for &(i, memory_value) in &memory {
if i == addr {
// prints the address and memory value at that address.
print_mem_address(addr, memory_value);
// sets the flag to true as the address has been initialized.
Expand Down Expand Up @@ -305,7 +305,7 @@ pub fn start_repl(library_paths: &Vec<PathBuf>, use_stdlib: bool) {
fn execute(
program: String,
provided_libraries: &[Library],
) -> Result<(Vec<(u64, Word)>, Vec<Felt>), String> {
) -> Result<(Vec<(u64, Felt)>, Vec<Felt>), String> {
// compile program
let mut assembler = Assembler::default();

Expand Down Expand Up @@ -404,7 +404,6 @@ fn print_stack(stack: Vec<Felt>) {

/// Accepts and returns a memory at an address by converting its register into integer
/// from Felt.
fn print_mem_address(addr: u64, mem: &Word) {
let mem_int = mem.iter().map(|&x| x.as_int()).collect::<Vec<_>>();
println!("{} {:?}", addr, mem_int)
fn print_mem_address(addr: u64, mem_value: Felt) {
println!("{addr} {mem_value}")
}
50 changes: 10 additions & 40 deletions miden/tests/integration/exec_iters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use vm_core::{debuginfo::Location, AssemblyOp, Operation};
// EXEC ITER TESTS
// =================================================================
/// TODO: Reenable (and fix) after we stabilized the assembler
/// Note: expect the memory values to be very wrong.
#[test]
#[ignore]
fn test_exec_iter() {
Expand All @@ -18,7 +19,8 @@ fn test_exec_iter() {
let traces = test.execute_iter();
let fmp = Felt::new(2u64.pow(30));
let next_fmp = fmp + ONE;
let mem = vec![(1_u64, slice_to_word(&[13, 14, 15, 16]))];
// TODO: double check this value
let mem = vec![(1_u64, Felt::from(13_u32))];
let mem_storew1_loc = Some(Location {
path: path.clone(),
start: 33.into(),
Expand Down Expand Up @@ -309,10 +311,7 @@ fn test_exec_iter() {
)),
stack: [17, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0].to_elements(),
fmp: next_fmp,
memory: vec![
(1_u64, slice_to_word(&[13, 14, 15, 16])),
(2u64.pow(30) + 1, slice_to_word(&[17, 0, 0, 0])),
],
memory: vec![(1_u64, 13_u32.into()), (2u64.pow(30) + 1, 17_u32.into())],
},
VmState {
clk: RowIndex::from(19),
Expand All @@ -330,10 +329,7 @@ fn test_exec_iter() {
)),
stack: [12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0, 0].to_elements(),
fmp: next_fmp,
memory: vec![
(1_u64, slice_to_word(&[13, 14, 15, 16])),
(2u64.pow(30) + 1, slice_to_word(&[17, 0, 0, 0])),
],
memory: vec![(1_u64, 13_u32.into()), (2u64.pow(30) + 1, 17_u32.into())],
},
VmState {
clk: RowIndex::from(20),
Expand All @@ -343,10 +339,7 @@ fn test_exec_iter() {
stack: [18446744069414584320, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0]
.to_elements(),
fmp: next_fmp,
memory: vec![
(1_u64, slice_to_word(&[13, 14, 15, 16])),
(2u64.pow(30) + 1, slice_to_word(&[17, 0, 0, 0])),
],
memory: vec![(1_u64, 13_u32.into()), (2u64.pow(30) + 1, 17_u32.into())],
},
VmState {
clk: RowIndex::from(21),
Expand All @@ -355,10 +348,7 @@ fn test_exec_iter() {
asmop: None,
stack: [12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0].to_elements(),
fmp,
memory: vec![
(1_u64, slice_to_word(&[13, 14, 15, 16])),
(2u64.pow(30) + 1, slice_to_word(&[17, 0, 0, 0])),
],
memory: vec![(1_u64, 13_u32.into()), (2u64.pow(30) + 1, 17_u32.into())],
},
VmState {
clk: RowIndex::from(22),
Expand All @@ -367,10 +357,7 @@ fn test_exec_iter() {
asmop: None,
stack: [12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0].to_elements(),
fmp,
memory: vec![
(1_u64, slice_to_word(&[13, 14, 15, 16])),
(2u64.pow(30) + 1, slice_to_word(&[17, 0, 0, 0])),
],
memory: vec![(1_u64, 13_u32.into()), (2u64.pow(30) + 1, 17_u32.into())],
},
VmState {
clk: RowIndex::from(23),
Expand All @@ -379,10 +366,7 @@ fn test_exec_iter() {
asmop: None,
stack: [12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0].to_elements(),
fmp,
memory: vec![
(1_u64, slice_to_word(&[13, 14, 15, 16])),
(2u64.pow(30) + 1, slice_to_word(&[17, 0, 0, 0])),
],
memory: vec![(1_u64, 13_u32.into()), (2u64.pow(30) + 1, 17_u32.into())],
},
VmState {
clk: RowIndex::from(24),
Expand All @@ -391,25 +375,11 @@ fn test_exec_iter() {
asmop: None,
stack: [12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0].to_elements(),
fmp,
memory: vec![
(1_u64, slice_to_word(&[13, 14, 15, 16])),
(2u64.pow(30) + 1, slice_to_word(&[17, 0, 0, 0])),
],
memory: vec![(1_u64, 13_u32.into()), (2u64.pow(30) + 1, 17_u32.into())],
},
];
for (expected, t) in expected_states.iter().zip(traces) {
let state = t.as_ref().unwrap();
assert_eq!(*expected, *state);
}
}

// HELPER FUNCTIONS
// =================================================================
fn slice_to_word(values: &[i32]) -> [Felt; 4] {
[
Felt::new(values[0] as u64),
Felt::new(values[1] as u64),
Felt::new(values[2] as u64),
Felt::new(values[3] as u64),
]
}
29 changes: 15 additions & 14 deletions processor/src/chiplets/memory/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use alloc::{collections::BTreeMap, vec::Vec};

use miden_air::{
trace::chiplets::memory::{
ADDR_COL_IDX, CLK_COL_IDX, CTX_COL_IDX, D0_COL_IDX, D1_COL_IDX, D_INV_COL_IDX,
BATCH_COL_IDX, CLK_COL_IDX, CTX_COL_IDX, D0_COL_IDX, D1_COL_IDX, D_INV_COL_IDX,
ELEMENT_OR_WORD_COL_IDX, FLAG_SAME_BATCH_AND_CONTEXT, IDX0_COL_IDX, IDX1_COL_IDX,
MEMORY_ACCESS_ELEMENT, MEMORY_ACCESS_WORD, MEMORY_READ, MEMORY_WRITE, READ_WRITE_COL_IDX,
V_COL_RANGE,
Expand Down Expand Up @@ -109,26 +109,27 @@ impl Memory {
///
/// Unlike read() which modifies the memory access trace, this method returns the value at the
/// specified address (if one exists) without altering the memory access trace.
pub fn get_value(&self, ctx: ContextId, addr: Felt) -> Option<Word> {
pub fn get_value(&self, ctx: ContextId, addr: Felt) -> Option<Felt> {
match self.trace.get(&ctx) {
// TODO(plafer): fix
// TODO(plafer): change `addr` to u32
Some(segment) => segment.get_value(addr.try_into().unwrap()),
None => None,
}
}

/// Returns the word at the specified context/address which should be used as the "old value"
/// for a write request. It will be the previously stored value, if one exists, or
/// initialized memory.
pub fn get_old_value(&self, ctx: ContextId, addr: Felt) -> Word {
// get the stored word or return [0, 0, 0, 0], since the memory is initialized with zeros
self.get_value(ctx, addr).unwrap_or(INIT_MEM_VALUE)
// TODO(plafer): is it ok to panic when addr is not aligned?
pub fn get_word(&self, ctx: ContextId, addr: Felt) -> Option<Word> {
match self.trace.get(&ctx) {
// TODO(plafer): change `addr` to u32
Some(segment) => segment.get_word(addr.try_into().unwrap()),
None => None,
}
}

/// Returns the entire memory state for the specified execution context at the specified cycle.
/// The state is returned as a vector of (address, value) tuples, and includes addresses which
/// have been accessed at least once.
pub fn get_state_at(&self, ctx: ContextId, clk: RowIndex) -> Vec<(u64, Word)> {
pub fn get_state_at(&self, ctx: ContextId, clk: RowIndex) -> Vec<(u64, Felt)> {
if clk == 0 {
return vec![];
}
Expand Down Expand Up @@ -338,7 +339,7 @@ impl Memory {
},
};
trace.set(row, CTX_COL_IDX, ctx);
trace.set(row, ADDR_COL_IDX, felt_addr);
trace.set(row, BATCH_COL_IDX, felt_addr);
trace.set(row, IDX0_COL_IDX, idx0);
trace.set(row, IDX1_COL_IDX, idx1);
trace.set(row, CLK_COL_IDX, clk);
Expand Down Expand Up @@ -396,9 +397,9 @@ impl Memory {
// TEST HELPERS
// --------------------------------------------------------------------------------------------

/// Returns current size of the memory (in words) across all contexts.
/// Returns the number of batches that were accessed at least once across all contexts.
#[cfg(test)]
pub fn size(&self) -> usize {
self.trace.iter().fold(0, |acc, (_, s)| acc + s.size())
pub fn num_accessed_batches(&self) -> usize {
self.trace.iter().fold(0, |acc, (_, s)| acc + s.num_accessed_batches())
}
}
Loading

0 comments on commit 6eadbd9

Please sign in to comment.