Spec: unified MEMMOVE accelerator - #986
nicole-graus wants to merge 11 commits into
Conversation
Kimi Code ReviewAutomated review by Kimi (Moonshot AI) |
Codex Code Review
|
Kimi Code ReviewAutomated review by Kimi (Moonshot AI) |
Codex Code Review
|
erik-3milabs
left a comment
There was a problem hiding this comment.
First set of comments. Primary change request: the COMMIT chip should be completely integrated into the MEMMOVE chip; no COMMIT chip should be needed after this PR is done.
Once this is done, I'd be happy to give this another review!
There was a problem hiding this comment.
The commit chip can be integrated in the MEMMOVE chip entirely: use the is_commit flag to select the right ecall code (feel free to update the commit ecall nr if required).
Also, note that memmove/memcpy/memset all return a void*, i.e., an address of sort. The same write statement can be used to return the proper return value for the commit chip.
There was a problem hiding this comment.
Yes, we can do that. We kept it because Robin suggested keeping it for the ecall number and the x254 update. We can integrate it, the total number of columns will shrink but the cost per-row of MEMMOVE goes up (COMMITS interactions will be paid by all the rows of MEMMOVE).
| [[variables.auxiliary]] | ||
| name = "tail" | ||
| type = "Bit" | ||
| desc = "Whether this row moves a single byte rather than eight" | ||
| pad = 1 | ||
|
|
||
| [[variables.auxiliary]] | ||
| name = "count_lt8" | ||
| type = "Bit" | ||
| desc = "Whether $#`count` < 8$, i.e. whether this row may not move eight bytes" | ||
| pad = 1 |
There was a problem hiding this comment.
consider merging these two variables into, e.g., "single".
Note that count < 8 => tail = 0 is equivalent to tail = 1 => count >= 8. This means that we can transform the link between them into
LT[single; count, 8] with multiplicity mu-single.
This would require introducing a single = 1 => mu = 1 constraint to ensure there is no negative multiplicity here.
There was a problem hiding this comment.
You're right. I will implement it
| [[constraints.width]] | ||
| kind = "interaction" | ||
| tag = "ALU" | ||
| input = ["count", ["cast", 257, "DWordWL"], ["opsel", "LT"]] | ||
| output = ["arr", 1, 0] | ||
| multiplicity = "first_ecall" | ||
| ref = "memmove:c:bound" |
There was a problem hiding this comment.
why are we restricting ourselves to count < 257, again?
In fact, why would we even need to range check count in the first place? Given that it is read from memory, we can assume it is a properly formatted value, and we use the SUB chip to decrement it appropriately, which also requires the decremented value to be range checked.
Recommendation: drop the constraint all together.
There was a problem hiding this comment.
We want to avoid one ecall to add a lot of rows and make an epoch unprovable. We kept that restriction from #874. Do you say it isn't a problem to have many rows for one cycle?
| [[constraints.commit]] | ||
| kind = "interaction" | ||
| tag = "memory" | ||
| input = [2, ["arr", ["+", ["idx", "dst", 0], "i"], ["idx", "dst", 1]], 0, ["idx", "value", "i"]] | ||
| iter = ["i", 0, 7] | ||
| multiplicity = ["idx", "commit_lane", "i"] | ||
| ref = "memmove:c:commit_value_out" | ||
|
|
||
| [[constraints.commit]] | ||
| kind = "interaction" | ||
| tag = "memory" | ||
| input = [2, ["arr", ["+", ["idx", "dst", 0], "i"], ["idx", "dst", 1]], 1, ["idx", "value", "i"]] | ||
| iter = ["i", 0, 7] | ||
| multiplicity = ["-", ["idx", "commit_lane", "i"]] | ||
| ref = "memmove:c:commit_value_in" |
There was a problem hiding this comment.
what is the reason you haven't used MEMW here? Doing so should reduce this chip considerably, as we reduce 16 interactions to 2.
There was a problem hiding this comment.
you're right, I will change it.
| [[constraints.read_input]] | ||
| kind = "template" | ||
| tag = "REG" | ||
| input = [10, "dst", "timestamp"] | ||
| output = "dst" | ||
| cond = "first_ecall" | ||
| ref = "memmove:c:read_dst" | ||
|
|
||
| [[constraints.read_input]] | ||
| kind = "template" | ||
| tag = "REG" | ||
| input = [11, "src", "timestamp"] | ||
| output = "src" | ||
| cond = "first_ecall" | ||
| ref = "memmove:c:read_src" | ||
|
|
||
| [[constraints.read_input]] | ||
| kind = "template" | ||
| tag = "REG" | ||
| input = [12, "count", "timestamp"] | ||
| output = "count" | ||
| cond = "first_ecall" | ||
| ref = "memmove:c:read_count" |
There was a problem hiding this comment.
what definition of memset are you using here? The one I found doesn't have a src, yet here we are reading it from 11.
There was a problem hiding this comment.
Yes, I agree. src is where the stub writes the bytes, since the accelerator doesn't fill just copies those bytes. But since it's always dst − 8 we can derive it instead of passing it, and then that constraint becomes a definition rather than a check.
There was a problem hiding this comment.
I was changing it in the code and realized that if we don't include src and we don't read it from x11, then the multiplicity should be f_ncommit * (1 − is_set) with a degree 2, since that read already has multiplicity f_ncommit and we would need to switch it off for memset. And a degree 2 would need an other column.
| [[constraints.incr_decr]] | ||
| kind = "template" | ||
| tag = "ADDNW" | ||
| input = ["src", ["arr", "step", 0]] | ||
| output = ["cast", "src_incr", "DWordWL"] | ||
| cond = ["-", "μ", "end"] | ||
| ref = "memmove:c:src_incr" |
There was a problem hiding this comment.
Note that by inlining the ADD chip, we can even reconstruct src_incr[3] from src_incr[2], src[1] and carry[0] since carry[1] = 0. This saves a column.
Same applies to dst_incr.
There was a problem hiding this comment.
True, I'll change it.
There was a problem hiding this comment.
Changed it in e43cad6. Note that src_incr[3] is used in the address we send on MEMMOVE_NEXT bus, so now we have to derive it there, making it a bit messier.
|
|
||
| These concern the _first_ row of a sequence, where the values come from the register file or from `COMMIT`; every later row receives them over `MEMMOVE_NEXT`, where @memmove:c:range_src_incr, @memmove:c:range_dst_incr and @memmove:c:range_count_decr range-check three of the four on the sending side. | ||
| `timestamp` is range-checked by neither side and holds only because it travels unchanged from the `ECALL` at the root. | ||
| @memmove:a:dst is not discharged at all on a commitment sequence (@memmove:aside:index). |
There was a problem hiding this comment.
what does it mean to "discharge" an assumption?
There was a problem hiding this comment.
We meant that an assumption is discharged when you can point to something that actually guarantees it. In that line we wanted to say that nothing guarantees it.
I agree it is confusing, I will rephrase it.
|
|
||
| == Selecting the functionality | ||
| @memmove:c:receive_ecall receives the system call number as $2^32 - 30 - 2 dot #`is_set`$, so `is_set` is decoded from the `ECALL` the guest executed rather than chosen. | ||
| Note that the low limb of that tuple is a line in `is_set` and so reaches every system call number in the negative range: @memmove:c:range_is_set is what excludes them, and it therefore carries the whole decoding argument. |
There was a problem hiding this comment.
... is a line in ...
what does that mean?
There was a problem hiding this comment.
A linear function in is_set. I will reword it
|
|
||
| = The Accelerated Memory Operations standard | ||
| The Ethereum Foundation's Accelerated Memory Operations standard fixes what an accelerated `memcpy`, `memmove` and `memset` must provide. | ||
| #footnote([Accelerated Memory Operations; eth-act/zkevm-standards, commit `e6a4cc0`. #link("https://github.com/eth-act/zkevm-standards/tree/e6a4cc0/standards/accelerated-memory-operations")[[src]]]) |
Co-authored-by: Erik <159244975+erik-3milabs@users.noreply.github.com>
…m the functionality selector
Description
Specifies the accelerator implemented in #984.
MEMMOVEchapter coveringmemcpy,memmove,memsetandcommit. The destination domain and the read/write timestamp order are decoded from the entry point, never chosen by the caller:is_setfrom the syscall number,is_commitfrom which bus the first row accepted from, both carried insideMEMMOVE_NEXTso a sequence cannot change functionality midway.COMMITis now one row per ecall: it keeps the syscall number and thex254update, and defers the byte loop overCOMMIT_DEFER.CNBis retired.ADDNWtemplate, which is what rules out a sequence closing into a ring.-30(copy) and-32(memset), and lists-31as reserved forHINTso it is not handed out twice.