Remove extra SHL/SHR CTL. #1270

LindaGuiga · 2023-10-02T22:57:09Z

This PR changes the way SHL and SHR are generated so that all the CPU CTLs looking into ArithmeticStark have the same form, and we can save one CTL.

Here, shift operations are generated on the arithmetic side rather on the CPU side. This leads to specific constraints for the shift operations, since now, the first input is shift rather than 1 << shift.
The three input registers are used in the Arithmetic STARK: the first is shift, the second is the input, and the third is 1 << shift.
This way, we can have the same CTL shape for all arithmetic operations, and it removes the special shift CTL.

@unzvfu Would mind looking at this if you have the time?

unzvfu

Overall this is a nice improvement, thanks! There are several things that will need fixing or tidying up though; see my comments.

unzvfu · 2023-10-04T11:38:13Z

evm/src/arithmetic/mod.rs

+                let shifted_input1 = if input1.bits() <= 32 {
+                    U256::one() << input1
+                } else {
+                    U256::zero()
+                };
+                input0.overflowing_mul(shifted_input1).0


Hmm, are you sure this is right? Maybe I'm missing something, but I would have expected this to be

Suggested change

let shifted_input1 = if input1.bits() <= 32 {

U256::one() << input1

} else {

U256::zero()

};

input0.overflowing_mul(shifted_input1).0

if input0 < U256::from(256usize) {

input1 << input0

} else {

U256::zero()

}

as in

plonky2/evm/src/cpu/kernel/interpreter.rs

Line 673 in 51eb7c0

fn run_shl(&mut self) {

If your implementation is correct, could you please document (i) why the argument order is reversed (cf. https://www.evm.codes/#1b?fork=shanghai) and (ii) why you limit input1 to 2^32 rather than 256? Thanks!

On the other hand, if my suggestion is correct, then we should probably expand the test suite since it would be a bit concerning that this wasn't caught!

The argument order for the generation on the Arithmetic side was indeed reversed: I fixed this.
I limited input1 to 2^32 bits (mistakenly) basing my code on this piece of code in operation.rs:

if input0.bits() <= 32 { let (_, read) = mem_read_gp_with_log_and_fill(LOOKUP_CHANNEL, lookup_addr, state, &mut row); state.traces.push_memory(read); } else { // The shift constraints still expect the address to be set, even though no read will occur. let channel = &mut row.mem_channels[LOOKUP_CHANNEL]; channel.addr_context = F::from_canonical_usize(lookup_addr.context); channel.addr_segment = F::from_canonical_usize(lookup_addr.segment); channel.addr_virtual = F::from_canonical_usize(lookup_addr.virt); }

where we carry out a memory read if the number of bits is less than 32. I changed the limitation to 16 bits in the new version, but I was wondering why there was a difference between the memory reads and the limit in shift?

unzvfu · 2023-10-04T11:45:46Z

evm/src/arithmetic/mod.rs

+                let shifted_input1 = if input1.bits() <= 32 {
+                    U256::one() << input1
+                } else {
+                    U256::zero()
+                };
+                if shifted_input1.is_zero() {
+                    U256::zero()
+                } else {
+                    input0 / shifted_input1
+                }


Similar concern to BinaryOperator::Shl above. I would have expected this to be

Suggested change

let shifted_input1 = if input1.bits() <= 32 {

U256::one() << input1

} else {

U256::zero()

};

if shifted_input1.is_zero() {

U256::zero()

} else {

input0 / shifted_input1

}

input1 >> input0

as in

plonky2/evm/src/cpu/kernel/interpreter.rs

Line 683 in 51eb7c0

fn run_shr(&mut self) {

Note that, in any case, we can just use the U256 arithmetic operations here rather than mimicking the way the values are verified later (e.g. shifted_input1 is not used elsewhere).

evm/src/arithmetic/mod.rs

unzvfu · 2023-10-04T12:07:47Z

evm/src/arithmetic/shift.rs

+    // The second register holds the input which needs shifting.
+    u256_to_array(&mut lv[INPUT_REGISTER_1], input);
+    u256_to_array(&mut lv[OUTPUT_REGISTER], result);
+    // If `shift > 2^32`, the shifted displacement is set to 0.


Suggested change

// If `shift > 2^32`, the shifted displacement is set to 0.

// If `shift >= 256`, the shifted displacement is set to 0.

evm/src/arithmetic/shift.rs

unzvfu · 2023-10-04T12:24:50Z

evm/src/arithmetic/shift.rs

+        lv[IS_SHR] = F::ZERO;
+
+        for _i in 0..N_RND_TESTS {
+            let shift = U256::from(rng.gen::<usize>());


If shift is a random usize-sized value, then the probability that it is less than 256 is about 2^-56. :)

Suggested change

let shift = U256::from(rng.gen::<usize>());

let shift = U256::from(rng.gen::<u8>());

Probably better to have a separate test for big shifts where the expected result is zero.

evm/src/arithmetic/shift.rs

unzvfu · 2023-10-04T12:37:06Z

evm/src/arithmetic/shift.rs

+                    U256::from(lv[ai].to_canonical_u64()) + full_input * U256::from(1 << 16);
+            }
+
+            let output = full_input * shifted;


Note that making the suggested change of usize to u8 above triggers an overflow exception on this line. Probably easiest to just do

Suggested change

let output = full_input * shifted;

let output = full_input << shift;

…est. Fix max number of bit shifts. Cleanup.

LindaGuiga · 2023-10-04T22:11:08Z

@unzvfu Thank you for your review!

As suggested in one of the comments, I added separate tests for shift > 256, and I realized a mistake had been introduced (I think in PR #1166, which introduced IS_SHR and IS_SHL) for that case: the modulus was not set to 0 but to 1 in that case (and some constraints involved only IS_DIV when they should have included both IS_DIV and IS_SHR).
I fixed this issue in the latest commit, where I also introduced the tests for shift > 256. If you think this should go into another PR, I can always revert the latest commit!

LindaGuiga · 2023-10-04T23:26:34Z

Just a side note on my latest commit and comment: the shift issue on main was causing a couple of tests to fail in evm-tests (shr_-1_256 / shr_2^255_256 / shr_2^255_257/expPower256Of256 for instance) and the tests pass with the fix.

unzvfu

Looks great, thanks Linda! Just a few little nits about function visibility that will be easy to incorporate (not a big deal, but I think its a good habit to limit the visibility of helper functions).

And nice work catching the bug with big shifts!

unzvfu · 2023-10-05T07:52:08Z

evm/src/arithmetic/divmod.rs


 /// Verify that num = quo * den + rem and 0 <= rem < den.
-fn eval_packed_divmod_helper<P: PackedField>(
+pub fn eval_packed_divmod_helper<P: PackedField>(


Nit:

Suggested change

pub fn eval_packed_divmod_helper<P: PackedField>(

pub(crate) fn eval_packed_divmod_helper<P: PackedField>(

unzvfu · 2023-10-05T07:55:10Z

evm/src/arithmetic/mul.rs

-    let input1 = read_value_i64_limbs(lv, INPUT_REGISTER_1);
-
+/// Given the two limbs of `left_in` and `right_in`, computes `left_in * right_in`.
+pub fn generate_mul<F: PrimeField64>(lv: &mut [F], left_in: [i64; 16], right_in: [i64; 16]) {


Nit:

Suggested change

pub fn generate_mul<F: PrimeField64>(lv: &mut [F], left_in: [i64; 16], right_in: [i64; 16]) {

pub(crate) fn generate_mul<F: PrimeField64>(lv: &mut [F], left_in: [i64; 16], right_in: [i64; 16]) {

unzvfu · 2023-10-05T07:56:49Z

evm/src/arithmetic/mul.rs

+    eval_packed_generic_mul(lv, is_mul, input0_limbs, input1_limbs, yield_constr);
+}
+
+pub fn eval_ext_mul_circuit<F: RichField + Extendable<D>, const D: usize>(


Nit:

Suggested change

pub fn eval_ext_mul_circuit<F: RichField + Extendable<D>, const D: usize>(

pub(crate) fn eval_ext_mul_circuit<F: RichField + Extendable<D>, const D: usize>(

unzvfu · 2023-10-05T07:57:22Z

evm/src/arithmetic/mul.rs

+    generate_mul(lv, input0, input1);
+}
+
+pub fn eval_packed_generic_mul<P: PackedField>(


Nit:

Suggested change

pub fn eval_packed_generic_mul<P: PackedField>(

pub(crate) fn eval_packed_generic_mul<P: PackedField>(

unzvfu · 2023-10-05T08:01:02Z

evm/src/arithmetic/shift.rs

+/// The logic is the same as the one for MUL. The only difference is that
+/// the inputs are in `INPUT_REGISTER_1`  and `INPUT_REGISTER_2` instead of
+/// `INPUT_REGISTER_0` and `INPUT_REGISTER_1`.
+pub fn eval_packed_shl<P: PackedField>(


Nit:

Suggested change

pub fn eval_packed_shl<P: PackedField>(

fn eval_packed_shl<P: PackedField>(

unzvfu · 2023-10-05T08:01:23Z

evm/src/arithmetic/shift.rs

+    eval_packed_shr(lv, nv, yield_constr);
+}
+
+pub fn eval_ext_circuit_shl<F: RichField + Extendable<D>, const D: usize>(


Nit:

Suggested change

pub fn eval_ext_circuit_shl<F: RichField + Extendable<D>, const D: usize>(

fn eval_ext_circuit_shl<F: RichField + Extendable<D>, const D: usize>(

unzvfu · 2023-10-05T08:01:37Z

evm/src/arithmetic/shift.rs

+    );
+}
+
+pub fn eval_ext_circuit_shr<F: RichField + Extendable<D>, const D: usize>(


Nit:

Suggested change

pub fn eval_ext_circuit_shr<F: RichField + Extendable<D>, const D: usize>(

fn eval_ext_circuit_shr<F: RichField + Extendable<D>, const D: usize>(

Remove extra shift CTL.

a1230f7

unzvfu self-requested a review October 3, 2023 06:24

unzvfu requested changes Oct 4, 2023

View reviewed changes

LindaGuiga added 2 commits October 4, 2023 17:58

Change order of inputs for the arithmetic shift operations. Add SHR t…

0f6404a

…est. Fix max number of bit shifts. Cleanup.

Fix SHR in the case shift >= 256

39dc14c

unzvfu approved these changes Oct 5, 2023

View reviewed changes

Limit visibility of helper functions

e8e050b

LindaGuiga merged commit 0de6f94 into 0xPolygonZero:main Oct 5, 2023
2 checks passed

Nashtare deleted the remove-shift-ctl branch January 9, 2024 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove extra SHL/SHR CTL. #1270

Remove extra SHL/SHR CTL. #1270

LindaGuiga commented Oct 2, 2023 •

edited

Loading

unzvfu left a comment

unzvfu Oct 4, 2023 •

edited

Loading

LindaGuiga Oct 4, 2023

unzvfu Oct 4, 2023

unzvfu Oct 4, 2023

unzvfu Oct 4, 2023

unzvfu Oct 4, 2023

LindaGuiga commented Oct 4, 2023 •

edited

Loading

LindaGuiga commented Oct 4, 2023 •

edited by Nashtare

Loading

unzvfu left a comment

unzvfu Oct 5, 2023

unzvfu Oct 5, 2023

unzvfu Oct 5, 2023

unzvfu Oct 5, 2023

unzvfu Oct 5, 2023

unzvfu Oct 5, 2023

unzvfu Oct 5, 2023

	// If `shift > 2^32`, the shifted displacement is set to 0.
	// If `shift >= 256`, the shifted displacement is set to 0.

	let shift = U256::from(rng.gen::<usize>());
	let shift = U256::from(rng.gen::<u8>());

	let output = full_input * shifted;
	let output = full_input << shift;

	pub fn eval_packed_divmod_helper<P: PackedField>(
	pub(crate) fn eval_packed_divmod_helper<P: PackedField>(

	pub fn generate_mul<F: PrimeField64>(lv: &mut [F], left_in: [i64; 16], right_in: [i64; 16]) {
	pub(crate) fn generate_mul<F: PrimeField64>(lv: &mut [F], left_in: [i64; 16], right_in: [i64; 16]) {

	pub fn eval_ext_mul_circuit<F: RichField + Extendable<D>, const D: usize>(
	pub(crate) fn eval_ext_mul_circuit<F: RichField + Extendable<D>, const D: usize>(

	pub fn eval_packed_generic_mul<P: PackedField>(
	pub(crate) fn eval_packed_generic_mul<P: PackedField>(

	pub fn eval_packed_shl<P: PackedField>(
	fn eval_packed_shl<P: PackedField>(

	pub fn eval_ext_circuit_shl<F: RichField + Extendable<D>, const D: usize>(
	fn eval_ext_circuit_shl<F: RichField + Extendable<D>, const D: usize>(

	pub fn eval_ext_circuit_shr<F: RichField + Extendable<D>, const D: usize>(
	fn eval_ext_circuit_shr<F: RichField + Extendable<D>, const D: usize>(

Remove extra SHL/SHR CTL. #1270

Remove extra SHL/SHR CTL. #1270

Conversation

LindaGuiga commented Oct 2, 2023 • edited Loading

unzvfu left a comment

Choose a reason for hiding this comment

unzvfu Oct 4, 2023 • edited Loading

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

LindaGuiga commented Oct 4, 2023 • edited Loading

LindaGuiga commented Oct 4, 2023 • edited by Nashtare Loading

unzvfu 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

LindaGuiga commented Oct 2, 2023 •

edited

Loading

unzvfu Oct 4, 2023 •

edited

Loading

LindaGuiga commented Oct 4, 2023 •

edited

Loading

LindaGuiga commented Oct 4, 2023 •

edited by Nashtare

Loading