-
Notifications
You must be signed in to change notification settings - Fork 308
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
feat(avm-mini): call and return opcodes #3704
Changes from 8 commits
d3b3ccd
77a5425
17057c8
abf939d
33984c9
64b5f10
cfe5b71
97480e1
75c772d
a607e9a
b392b2e
4e54074
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 | ||||
---|---|---|---|---|---|---|
|
@@ -7,6 +7,18 @@ namespace avmMini(256); | |||||
pol constant clk(i) { i }; | ||||||
pol constant first = [1] + [0]*; // Used mostly to toggle off the first row consisting | ||||||
// only in first element of shifted polynomials. | ||||||
|
||||||
//===== CONTROL FLOW ========================================================== | ||||||
// Program counter | ||||||
pol commit pc; | ||||||
// Return Pointer | ||||||
pol commit internal_return_ptr; | ||||||
|
||||||
pol commit sel_internal_call; | ||||||
pol commit sel_internal_return; | ||||||
|
||||||
// Halt program execution | ||||||
pol commit sel_halt; | ||||||
|
||||||
//===== TABLE SUBOP-TR ======================================================== | ||||||
// Boolean selectors for (sub-)operations. Only one operation is activated at | ||||||
|
@@ -61,6 +73,11 @@ namespace avmMini(256); | |||||
sel_op_sub * (1 - sel_op_sub) = 0; | ||||||
sel_op_mul * (1 - sel_op_mul) = 0; | ||||||
sel_op_div * (1 - sel_op_div) = 0; | ||||||
sel_op_div * (1 - sel_op_div) = 0; | ||||||
|
||||||
sel_internal_call * (1 - sel_internal_call) = 0; | ||||||
sel_internal_return * (1 - sel_internal_return) = 0; | ||||||
sel_halt * (1 - sel_halt) = 0; | ||||||
|
||||||
op_err * (1 - op_err) = 0; | ||||||
|
||||||
|
@@ -108,4 +125,32 @@ namespace avmMini(256); | |||||
// Same for the relations related to the error activation: | ||||||
// (ib * inv - 1 + op_div_err) = 0 && op_err * (1 - inv) = 0 | ||||||
// This works in combination with op_div_err * (sel_op_div - 1) = 0; | ||||||
// Drawback is the need to paralllelize the latter. | ||||||
// Drawback is the need to paralllelize the latter. | ||||||
|
||||||
|
||||||
//===== CALL_RETURN ======================================================== | ||||||
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. do i need to assert the internal call counter does not change between calls? resolved |
||||||
// The program counter in the next row should be equal to the value loaded from the ia register | ||||||
// This implies that a load from memory must occur at the same time | ||||||
// Imply that we must load the jump selector into mema | ||||||
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. @Maddiaa0 mema is not a defined term. Did you mean mem_idx_a? 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. yes! updated |
||||||
#[return_pointer_increment] | ||||||
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. @Maddiaa0 All identifiers for relations in my PR are capitalized. I think it helps with visibility. At least we should agree on using the same notation. 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. done |
||||||
sel_internal_call * ( internal_return_ptr' - ( internal_return_ptr + 1)) = 0; // we increment our jump selector | ||||||
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. @Maddiaa0 We might use another term than "jump selector" as the operations are not named JUMP anymore. |
||||||
sel_internal_call * ( internal_return_ptr - mem_idx_a) = 0; | ||||||
sel_internal_call * (rwa - 1) = 0; // it must be a write | ||||||
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. @Maddiaa0 In the original design, it was meant that there would be a fixed tuple lookup mapping an operation to the corresponding sub-operations included load/store. In this case, relations
would not be required. I did not introduce such ones for the arithmetic operations. 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. Oh awesome ! |
||||||
sel_internal_call * (mem_op_a - 1) = 0; | ||||||
sel_internal_call * ((pc + 1) - ia) = 0; | ||||||
|
||||||
// We must load the memory pointer to be the internal_return_ptr | ||||||
#[return_pointer_decrement] | ||||||
sel_internal_return * ( internal_return_ptr' - ( internal_return_ptr - 1)) = 0; // We decrement out jump selector | ||||||
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.
Suggested change
|
||||||
sel_internal_return * ( (internal_return_ptr - 1) - mem_idx_a) = 0; | ||||||
sel_internal_return * (pc' - ia) = 0; | ||||||
sel_internal_return * rwa = 0; | ||||||
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 remark as above related to the OP-SUBOP table. |
||||||
sel_internal_return * (mem_op_a - 1) = 0; | ||||||
|
||||||
// Program counter must increment if not jumping or returning | ||||||
#[pc_increment] | ||||||
(1-first) * (1- sel_halt) * (sel_op_add + sel_op_sub + sel_op_div + sel_op_mul) * (pc' - (pc + 1)) = 0; | ||||||
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. @Maddiaa0 Please be consistent with spacing between arithmetic operations:
|
||||||
|
||||||
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. @Maddiaa0 Should not we have a constraint on internal_return_ptr value staying constant for all other operations? Otherwise, we could change maliciously the index between to internal_return/call, no? 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. #3704 (comment) yes! I wondered this. I shall implement it |
||||||
|
||||||
// TODO: we want to set an initial number for the reserved memory of the jump pointer | ||||||
|
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.
@Maddiaa0 Did you duplicate this line by mistake?
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.
good catch!