-
Notifications
You must be signed in to change notification settings - Fork 39
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: Add INCR
privileged instructions
#734
base: develop
Are you sure you want to change the base?
Changes from all commits
82c5982
cca978f
1270181
c69d9e9
b99ed9c
89caabf
a848075
b45751c
f444249
58cf528
7c765fd
b786905
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 | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,76 @@ | ||||||
use plonky2::field::extension::Extendable; | ||||||
use plonky2::field::packed::PackedField; | ||||||
use plonky2::field::types::Field; | ||||||
use plonky2::hash::hash_types::RichField; | ||||||
use plonky2::iop::ext_target::ExtensionTarget; | ||||||
use plonky2::plonk::circuit_builder::CircuitBuilder; | ||||||
use starky::constraint_consumer::{ConstraintConsumer, RecursiveConstraintConsumer}; | ||||||
|
||||||
use super::dup_swap::{constrain_channel_ext_circuit, constrain_channel_packed}; | ||||||
use crate::cpu::columns::CpuColumnsView; | ||||||
|
||||||
/// Evaluates the constraints for the DUP and SWAP opcodes. | ||||||
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. Update comment. 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
|
||||||
pub(crate) fn eval_packed<P: PackedField>( | ||||||
lv: &CpuColumnsView<P>, | ||||||
nv: &CpuColumnsView<P>, | ||||||
yield_constr: &mut ConstraintConsumer<P>, | ||||||
) { | ||||||
let filter = lv.op.incr; | ||||||
|
||||||
let n = lv.opcode_bits[0] | ||||||
+ lv.opcode_bits[1] * P::Scalar::from_canonical_u64(2) | ||||||
+ lv.opcode_bits[2] * P::Scalar::from_canonical_u64(4); | ||||||
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. I thought 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. The opcodes go from |
||||||
|
||||||
// Disable the partial channel. | ||||||
yield_constr.constraint(lv.op.incr * lv.partial_channel.used); | ||||||
|
||||||
// Constrain the input channel's address, `is_read` and `used` fields. | ||||||
let read_channel = &lv.mem_channels[1]; | ||||||
constrain_channel_packed(true, filter, n, read_channel, lv, yield_constr); | ||||||
|
||||||
// Constrain the output channel's address, `is_read` and `used` fields. | ||||||
let write_channel = &lv.mem_channels[2]; | ||||||
constrain_channel_packed(false, filter, n, write_channel, lv, yield_constr); | ||||||
|
||||||
// Constrain the unchanged stack len. | ||||||
yield_constr.constraint_transition(filter * (nv.stack_len - lv.stack_len)); | ||||||
} | ||||||
|
||||||
/// Circuit version of `eval_packed`. | ||||||
/// Evaluates the constraints for the DUP and SWAP opcodes. | ||||||
pub(crate) fn eval_ext_circuit<F: RichField + Extendable<D>, const D: usize>( | ||||||
builder: &mut CircuitBuilder<F, D>, | ||||||
lv: &CpuColumnsView<ExtensionTarget<D>>, | ||||||
nv: &CpuColumnsView<ExtensionTarget<D>>, | ||||||
yield_constr: &mut RecursiveConstraintConsumer<F, D>, | ||||||
) { | ||||||
let filter = lv.op.incr; | ||||||
|
||||||
let n = lv.opcode_bits[..3].iter().enumerate().fold( | ||||||
builder.zero_extension(), | ||||||
|cumul, (i, &bit)| { | ||||||
builder.mul_const_add_extension(F::from_canonical_u64(1 << i), bit, cumul) | ||||||
}, | ||||||
); | ||||||
|
||||||
// Disable the partial channel. | ||||||
{ | ||||||
let constr = builder.mul_extension(lv.op.incr, lv.partial_channel.used); | ||||||
yield_constr.constraint(builder, constr); | ||||||
} | ||||||
|
||||||
// Constrain the input channel's address, `is_read` and `used` fields. | ||||||
let read_channel = &lv.mem_channels[1]; | ||||||
constrain_channel_ext_circuit(builder, true, filter, n, read_channel, lv, yield_constr); | ||||||
|
||||||
// Constrain the output channel's address, `is_read` and `used` fields. | ||||||
let write_channel = &lv.mem_channels[2]; | ||||||
constrain_channel_ext_circuit(builder, false, filter, n, write_channel, lv, yield_constr); | ||||||
|
||||||
// Constrain the unchanged stack len. | ||||||
{ | ||||||
let diff = builder.sub_extension(nv.stack_len, lv.stack_len); | ||||||
let constr = builder.mul_extension(filter, diff); | ||||||
yield_constr.constraint_transition(builder, constr); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,7 @@ modmul_remainder_loop: | |
DUP4 ADD // out_addr_i | ||
%swap_mstore | ||
// stack: i, base_addr, len, a_loc, b_loc, m_loc, out_loc, s1, s2, s3, retdest | ||
%increment | ||
INCR1 | ||
DUP3 | ||
DUP2 | ||
// stack: i+1, len, i+1, base_addr, len, a_loc, b_loc, m_loc, out_loc, s1, s2, s3, retdest | ||
|
@@ -89,7 +89,7 @@ modmul_quotient_loop: | |
DUP4 ADD // s1_addr_i | ||
%swap_mstore | ||
// stack: i, base_addr, 2*len, len, a_loc, b_loc, m_loc, out_loc, s1, s2, s3, retdest | ||
%increment | ||
INCR1 | ||
DUP3 | ||
DUP2 | ||
// stack: i+1, 2*len, i+1, base_addr, 2*len, len, a_loc, b_loc, m_loc, out_loc, s1, s2, s3, retdest | ||
|
@@ -156,13 +156,12 @@ modmul_check_loop: | |
SWAP1 | ||
%decrement | ||
// stack: n-1, base_addr, i, j, retdest | ||
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. Probably faster to do:
|
||
INCR3 | ||
SWAP2 | ||
%increment | ||
// stack: i+1, base_addr, n-1, j, retdest | ||
SWAP3 | ||
%increment | ||
// stack: j+1, base_addr, n-1, i+1, retdest | ||
%stack (j, addr, n, i) -> (n, addr, n, i, j) | ||
INCR4 | ||
// stack: i+1, base_addr, n-1, j+1, retdest | ||
%stack (i, addr, n) -> (n, addr, n, i) | ||
// stack: n-1, base_addr, n-1, i+1, j+1, retdest | ||
%jumpi(modmul_check_loop) | ||
// end of modmul_check_loop | ||
|
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.
Some constraints seem to be missing, and some seem to be unneeded.
The current set of constraints work as intended for
INCR2-4
, but forINCR1
we are not checking that the output channel is equal to the next top of the stack.Moreover, the value read in the input channel is not constrained to match the current top of the stack (tests pass so it seems to be the case, but it sounds like coincidence to me).
I think the clean way to do it is to filter all of the current constraints with
lv.opcode_bits[0]
(with a new one making sure that the top of the stack doesn't change), and handle INCR1 separately with filter1 - lv.opcode_bits[0]
(you can even disable the memory channels to save some memory rows).