-
Notifications
You must be signed in to change notification settings - Fork 286
docs(memory): fix delta encoding comments in memory trace #2910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Changes from 6 commits
d01fbf9
15408d1
ba4373f
cade63d
f1e4280
ae57bc7
0f49aab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,13 +82,14 @@ const INIT_MEM_VALUE: Word = EMPTY_WORD; | |
| /// - When the context remains the same but the word changes, these columns contain (`new_word` | ||
| /// - `old_word`). | ||
| /// - When both the context and the word remain the same, these columns contain (`new_clk` - | ||
| /// `old_clk` - 1). | ||
| /// `old_clk`). | ||
| /// - `d_inv` contains the inverse of the delta between two consecutive context IDs, words, or clock | ||
| /// cycles computed as described above. It is the field inverse of `(d_1 * 2^16) + d_0` | ||
| /// - `f_scw` is a flag indicating whether the context and the word of the current row are the same | ||
| /// as in the next row. | ||
| /// as in the previous row. | ||
| /// | ||
| /// For the first row of the trace, values in `d0`, `d1`, and `d_inv` are set to zeros. | ||
| /// For the first row of the trace, `prev_clk` is initialized to `first_clk - 1`, so `d0`, | ||
| /// `d1`, and `d_inv` reflect the delta against that value rather than being forced to zero. | ||
| #[derive(Debug, Default)] | ||
| pub struct Memory { | ||
| /// Memory segment traces sorted by their execution context ID. | ||
|
|
@@ -248,8 +249,9 @@ impl Memory { | |
| /// [RangeChecker] chiplet instance, along with their row in the finalized execution trace. | ||
| pub fn append_range_checks(&self, memory_start_row: RowIndex, range: &mut RangeChecker) { | ||
| // set the previous address and clock cycle to the first address and clock cycle of the | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: should this be "initialize the previous..." |
||
| // trace; we also adjust the clock cycle so that delta value for the first row would end | ||
| // up being ZERO. if the trace is empty, return without any further processing. | ||
| // trace; we also adjust the clock cycle to be one less than the first row's clock so the | ||
| // first-row delta is computed against the immediately preceding clock. if the trace is | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means the delta is one right? Isn't that clearer? |
||
| // empty, return without any further processing. | ||
| let (mut prev_ctx, mut prev_addr, mut prev_clk) = match self.get_first_row_info() { | ||
| Some((ctx, addr, clk)) => (ctx, addr, clk.as_canonical_u64() - 1), | ||
| None => return, | ||
|
|
@@ -291,9 +293,10 @@ impl Memory { | |
| pub fn fill_trace(self, trace: &mut TraceFragment) { | ||
| debug_assert_eq!(self.trace_len(), trace.len(), "inconsistent trace lengths"); | ||
|
|
||
| // set the pervious address and clock cycle to the first address and clock cycle of the | ||
| // trace; we also adjust the clock cycle so that delta value for the first row would end | ||
| // up being ZERO. if the trace is empty, return without any further processing. | ||
| // set the previous address and clock cycle to the first address and clock cycle of the | ||
| // trace; we also adjust the clock cycle to be one less than the first row's clock so the | ||
| // first-row delta is computed against the immediately preceding clock. if the trace is | ||
| // empty, return without any further processing. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comments as above |
||
| let (mut prev_ctx, mut prev_addr, mut prev_clk) = match self.get_first_row_info() { | ||
| Some((ctx, addr, clk)) => (Felt::from(ctx), Felt::from_u32(addr), clk - ONE), | ||
| None => return, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the
old/newa bit confusing when it comes to the constraints, since we often use local/current and next for the two consecutive rows.I get the this trace is a bit special because we're treating the current row as the previous state and the next one as the current one, but maybe a better naming would be
prev/currto be a bit more consistent?Cc @Al-Kindi-0, @plafer