Handle additional memory requests made by `RCOMBBASE` #1229

Al-Kindi-0 · 2024-02-01T16:33:28Z

Describe your changes

Updates the bus to handle the two memory requests made by RCOMBBASE and updates docs.

Checklist before requesting a review

Repo forked and branch created from next according to naming convention.
Commit messages and codestyle follow conventions.
Relevant issues are linked in the PR description.
Tests added for new functionality.
Documentation/comments updated according to changes.

bobbinth

Thank you! Looks good! I rebased the PR from the latest code and left a few comments inline.

air/src/trace/main_trace.rs

docs/src/design/stack/crypto_ops.md

bobbinth · 2024-02-06T09:27:17Z

processor/src/chiplets/aux_trace/mod.rs

+    let factor1 = alphas[0]
+        + alphas[1].mul_base(Felt::from(op_label))
+        + alphas[2].mul_base(ctx)
+        + alphas[3].mul_base(z_ptr)
+        + alphas[4].mul_base(clk)
+        + alphas[5].mul_base(tz0)
+        + alphas[6].mul_base(tz1)
+        + alphas[7].mul_base(tzg0)
+        + alphas[8].mul_base(tzg1);
+
+    let factor2 = alphas[0]
+        + alphas[1].mul_base(Felt::from(op_label))
+        + alphas[2].mul_base(ctx)
+        + alphas[3].mul_base(a_ptr)
+        + alphas[4].mul_base(clk)
+        + alphas[5].mul_base(a0)
+        + alphas[6].mul_base(a1)
+        + alphas[7].mul_base(ZERO)
+        + alphas[8].mul_base(ZERO);


These two computations are very similar - I wonder if we should have a separate function for computing memory request values. For example, something like:

fn compute_memory_read_request(alphas: &[E], ctx: Felt, clk: Felt, addr: Felt, value: Word) -> E

(or maybe ctx and clk could be replaced with self if we put this function on MainTrace).

This will make the intent of the code a bit more clear. Also, such a function may be useful in other contexts too.

Agreed! I have now added the proposed method to MainTrace

plafer

Left a change proposal. Would love to hear your thoughts!

plafer · 2024-02-06T13:38:40Z

air/src/trace/main_trace.rs

-    pub fn helper_2(&self, i: usize) -> Felt {
-        self.columns.get_column(DECODER_TRACE_OFFSET + USER_OP_HELPERS_OFFSET + 2)[i]
+    /// The i-th decoder helper register at `row`.
+    pub fn helper(&self, i: usize, row: usize) -> Felt {


I would rename to helper_register() - it wasn't clear to me what main_trace.helper() was

Yes, makes sense. Renamed

plafer · 2024-02-06T16:54:06Z

air/src/trace/main_trace.rs

@@ -367,6 +357,30 @@ impl MainTrace {
        self.columns.get_column(MEMORY_V_COL_RANGE.start + 3)[i]
    }

+    /// Computes a memory read request at `row` given randomness `alphas`, memory address `addr`
+    /// and value `value`.
+    pub fn compute_memory_read_request<E: FieldElement<BaseField = Felt>>(


With this new function, we have 2 functions in the system that build a memory read request: this one, and processor::chiplets::aux_trace::build_mem_request(). We should have only one.

A few thoughts:

I think MainTrace should only have dumb accessors. It shouldn't be computing complex functions of the trace columns.

A memory read (for words) is always a random linear combination of [op_label, ctx, addr, clk, VALUE]. The issue we have is that MLOAD* and MSTORE* don't take these values from the same locations in the trace as RCOMBBASE. The solution IMO is to have build_mem_request take the values it needs as input, and caller can supply them by reading from the correct trace location.

I would split build_mem_request() into 2: one that builds a memory access for a Felt (e.g. build_mem_element_request()), and another for a Word (e.g. build_mem_word_request()). This would help us think of them as separate requests, because they are (i.e. the virtual columns that they collapse into 1 are different).

In other words, we currently think of a memory request (as defined by build_mem_request()) as coming from MLOAD* or MSTORE*. This change would help us think of a memory request as a random linear combination of specific columns (depending on whether we're reading a Felt or Word), which can come from any operation (current and/or future ones).

With this new function, we have 2 functions in the system that build a memory read request: this one, and processor::chiplets::aux_trace::build_mem_request().

Ah - good point! I forgot about this one. We actually also have build_mstream_request() which does something similar. I'd love to consolidate them to use the same logic for computing the random linear combination.

I think MainTrace should only have dumb accessors. It shouldn't be computing complex functions of the trace columns.

I agree with this. I wanted to have a more "convenient" location for the function - but separation of concerns is probably more important here.

As mentioned above, I still think that it would be good to have the function that computes the random linear combination for the request - but this function could probably live in the aux_trace module. Than, this function could be used from build_mem_request(), build_mstream_request(), build_rcomb_base_request() and others in the future.

I would split build_mem_request() into 2: one that builds a memory access for a Felt (e.g. build_mem_element_request()), and another for a Word (e.g. build_mem_word_request()). This would help us think of them as separate requests, because they are (i.e. the virtual columns that they collapse into 1 are different).

If we have the above-mentioned function as a helper, I think splitting up build_mem_request() would make sense.

I agree with points 1 and 2. Regarding point 3, it is indeed helpful (especially for the future) to make the code reflect the separation between requests (and by transitivity virtual columns). The issue is that this can be in conflict with reducing the size of the code in some places. Nevertheless, I have split the build_mem_request() and we can discuss the other cases, if any, on a case-by-case basis

plafer · 2024-02-06T16:54:59Z

processor/src/chiplets/aux_trace/mod.rs

+    let factor1 = main_trace.compute_memory_read_request(alphas, i, z_ptr, [tz0, tz1, tzg0, tzg1]);
+    let factor2 = main_trace.compute_memory_read_request(alphas, i, a_ptr, [a0, a1, ZERO, ZERO]);


Given previous comment, this would call the new build_mem_word_request()

Makes sense, changed!

plafer · 2024-02-06T16:57:19Z

air/src/trace/main_trace.rs

+        let ctx = self.ctx(row);
+        let clk = self.clk(row);
+
+        alphas[0]


Just a note: with #1223 we would take alphas[0] out of each memory request (such that each request is truly a random linear combination as opposed to an affine one). But we should leave as is in this PR, and change it only in #1223.

processor/src/chiplets/aux_trace/mod.rs

plafer · 2024-02-07T18:10:44Z

processor/src/chiplets/aux_trace/mod.rs

+        main_trace.helper_register(2, row),
+        main_trace.helper_register(1, row),
+        main_trace.helper_register(0, row),


What is stored in these helper registers, and why is it included as part of the request computation? Can we add a comment that explains it?

This would help understand the value argument to compute_memory_request(), which is currently unclear to me (since it's not always "the word that was read or written" EDIT - or is it?).

And based on this new understanding, I might rename build_mem_request_{element, word}().

I infer from build_memory_chiplet_response() that the helper registers must hold the remaining 3 elements that were read/written.

I would

make that clear in a comment here

In compute_memory_request()'s docstring, I would add a sentence to say that value is the memory value that was read/written

rename build_mem_request_element() -> build_loadstore_request()

rename build_mem_request_word() -> build_loadstorew_request()

I suggest renaming build_mem_request_{element, word}() yet again because I had something else in mind when I suggested it. I thought we would have these 2 basic functions that everything else would depend on (which is now compute_memory_request()). Now these 2 are associated exclusively with the MLOAD/STORE{W} instructions, and so I would rename them so as to capture that.

I think the naming is fine as it stands given the totality of the discussions above.

bobbinth

Looks good! Thank you! I left one comment for the future inline.

air/src/trace/main_trace.rs

plafer

Looks good :)

Al-Kindi-0 requested a review from bobbinth February 1, 2024 16:33

bobbinth force-pushed the al-simplify-aux branch from 5f36479 to df15de8 Compare February 6, 2024 08:37

bobbinth force-pushed the al-simplify-aux-rcomb branch from a54ba5a to 50f9395 Compare February 6, 2024 09:08

bobbinth requested a review from plafer February 6, 2024 09:13

bobbinth requested changes Feb 6, 2024

View reviewed changes

plafer requested changes Feb 6, 2024

View reviewed changes

plafer reviewed Feb 7, 2024

View reviewed changes

bobbinth mentioned this pull request Feb 8, 2024

Update code to be compatible with Winterfell 0.8 #1234

Merged

Al-Kindi-0 force-pushed the al-simplify-aux-rcomb branch from a5bf6a1 to 19df843 Compare February 8, 2024 08:23

bobbinth force-pushed the al-simplify-aux branch from 3d06daa to 4c963d2 Compare February 8, 2024 08:29

Al-Kindi-0 and others added 14 commits February 8, 2024 09:46

wip: add request rcomb_base

c0c49a5

chore: changed name of op and add docs

ab029d6

chore: minor nits and update figure

0b6594a

docs: add note on circular shift of top 8 elements

7d8f741

chore: rename variables in rcomb requests

9c389f1

wip

2b57098

fix: docs rcomb_base bus requests

637b5ae

chore: refactor MainTrace getter

8460aab

docs: remove duplicate comment paragraph

a41465d

chore: fix typos and add mem_request method

343e69d

chore: refactor memory requests

1f7f4c8

chore: rename to

bb5d477

chore: remove pub

3f0312b

doc: use uppercase

b56558e

Al-Kindi-0 force-pushed the al-simplify-aux-rcomb branch from 19df843 to b56558e Compare February 8, 2024 08:54

Al-Kindi-0 marked this pull request as ready for review February 8, 2024 08:56

bobbinth approved these changes Feb 8, 2024

View reviewed changes

air/src/trace/main_trace.rs Show resolved Hide resolved

Al-Kindi-0 mentioned this pull request Feb 8, 2024

Introduce a newtype for to represent a row in a trace #1236

Closed

plafer approved these changes Feb 8, 2024

View reviewed changes

bobbinth merged commit d8ae2e5 into al-simplify-aux Feb 8, 2024
15 checks passed

bobbinth deleted the al-simplify-aux-rcomb branch February 8, 2024 17:31

bobbinth pushed a commit that referenced this pull request Feb 8, 2024

Handle additional memory requests made by RCOMBBASE (#1229)

156a257

Fumuran pushed a commit that referenced this pull request Feb 9, 2024

Handle additional memory requests made by RCOMBBASE (#1229)

43382bd

bobbinth pushed a commit that referenced this pull request Feb 13, 2024

Handle additional memory requests made by RCOMBBASE (#1229)

df6d850

bobbinth mentioned this pull request Feb 26, 2024

Handle extra memory requests made by new RCombBase op #1222

Closed

bobbinth pushed a commit that referenced this pull request Feb 26, 2024

Handle additional memory requests made by RCOMBBASE (#1229)

9be1d3e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle additional memory requests made by `RCOMBBASE` #1229

Handle additional memory requests made by `RCOMBBASE` #1229

Al-Kindi-0 commented Feb 1, 2024

bobbinth left a comment

bobbinth Feb 6, 2024

Al-Kindi-0 Feb 6, 2024

plafer left a comment

plafer Feb 6, 2024

Al-Kindi-0 Feb 7, 2024

plafer Feb 6, 2024

bobbinth Feb 6, 2024

Al-Kindi-0 Feb 7, 2024

plafer Feb 6, 2024

Al-Kindi-0 Feb 7, 2024

plafer Feb 6, 2024

plafer Feb 7, 2024 •

edited

Loading

plafer Feb 7, 2024

Al-Kindi-0 Feb 8, 2024

bobbinth left a comment

plafer left a comment

		let factor1 = main_trace.compute_memory_read_request(alphas, i, z_ptr, [tz0, tz1, tzg0, tzg1]);
		let factor2 = main_trace.compute_memory_read_request(alphas, i, a_ptr, [a0, a1, ZERO, ZERO]);

Handle additional memory requests made by RCOMBBASE #1229

Handle additional memory requests made by RCOMBBASE #1229

Conversation

Al-Kindi-0 commented Feb 1, 2024

Describe your changes

Checklist before requesting a review

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Handle additional memory requests made by `RCOMBBASE` #1229

Handle additional memory requests made by `RCOMBBASE` #1229

plafer Feb 7, 2024 •

edited

Loading