Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implementing Shift instructions #41

Merged
merged 25 commits into from
Jun 27, 2024
Merged

Implementing Shift instructions #41

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e0a55ab
Init shift opcodes
jorbush Jun 20, 2024
1d9a56c
Test shl
jorbush Jun 21, 2024
0ce630c
Test shr
jorbush Jun 21, 2024
c6a3588
Rename file name declaration
jorbush Jun 21, 2024
a491ed3
Test shl using stack
jorbush Jun 21, 2024
cab2dcc
Test shr using stack
jorbush Jun 21, 2024
becdb34
Test shl and shr with eq flag
jorbush Jun 21, 2024
a1034eb
Format zasm
jorbush Jun 25, 2024
aa8ce95
Fix rol and test it
jorbush Jun 25, 2024
23d6274
Fix ror and test it
jorbush Jun 25, 2024
68d2e4b
Test rol and ror with stack
jorbush Jun 25, 2024
0aca3d2
Test rol and ror with conditional flag
jorbush Jun 25, 2024
2ea64de
Add eq flag testing
jorbush Jun 25, 2024
374a4f9
Format zasm
jorbush Jun 25, 2024
ff48a5d
Merge branch 'main' into shift-opcodes
jorbush Jun 25, 2024
53ce22d
Use TaggedValue
jorbush Jun 25, 2024
32cdd68
Remove unnecessary register setup
jorbush Jun 25, 2024
202aed3
Fix stack usage
jorbush Jun 25, 2024
ef108cf
Use unreacheable
jorbush Jun 26, 2024
9a4d853
Remove redundant cast
jorbush Jun 26, 2024
850ea74
Relocate shift match
jorbush Jun 26, 2024
48893a5
Remove 4 bytes redundancy
jorbush Jun 26, 2024
616b8a9
Test shift for number greater than 256
jorbush Jun 27, 2024
cf13900
Merge branch 'main' into shift-opcodes
jorbush Jun 27, 2024
d9f65a7
Add more test for values greater than 256
jorbush Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions programs/rol.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.text
.file "rol.zasm"
.globl __entry
__entry:
.func_begin0:
add 1, r0, r1
add 4, r0, r2
rol r1, r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
13 changes: 13 additions & 0 deletions programs/rol_conditional_eq.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.text
.file "rol_conditional_eq.zasm"
.globl __entry
__entry:
.func_begin0:
add 1, r0, r1
add 4, r0, r2
rol.eq r1, r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
16 changes: 16 additions & 0 deletions programs/rol_sets_eq_flag.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
.text
.file "rol_sets_eq_flag.zasm"
.globl __entry
__entry:

.func_begin0:
; EQ is set if the result is zero
add 0, r0, r1
add 0, r0, r2
rol! r1, r2, r1
sstore r0, r1
ret

.func_end0:
.note.GNU-stack
.rodata
17 changes: 17 additions & 0 deletions programs/rol_stack.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
.text
.file "rol_stack.zasm"
.globl __entry
__entry:
.func_begin0:
; grow stack
add 1, r0, stack+=[2]
; set stack value
add 1, r0, stack=[0]
jorbush marked this conversation as resolved.
Show resolved Hide resolved
add 4, r0, r2
; left rotation
rol stack=[0], r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
13 changes: 13 additions & 0 deletions programs/ror.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.text
.file "ror.zasm"
.globl __entry
__entry:
.func_begin0:
add 16, r0, r1
add 4, r0, r2
ror r1, r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
13 changes: 13 additions & 0 deletions programs/ror_conditional_eq.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.text
.file "ror_conditional_eq.zasm"
.globl __entry
__entry:
.func_begin0:
add 16, r0, r1
add 4, r0, r2
ror.eq r1, r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
16 changes: 16 additions & 0 deletions programs/ror_sets_eq_flag.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
.text
.file "ror_sets_eq_flag.zasm"
.globl __entry
__entry:

.func_begin0:
; EQ is set if the result is zero
add 0, r0, r1
add 0, r0, r2
ror! r1, r2, r1
sstore r0, r1
ret

.func_end0:
.note.GNU-stack
.rodata
17 changes: 17 additions & 0 deletions programs/ror_stack.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
.text
.file "ror_stack.zasm"
.globl __entry
__entry:
.func_begin0:
; grow stack
add 1, r0, stack+=[2]
; set stack value
add 16, r0, stack=[0]
add 4, r0, r2
; right rotation
ror stack=[0], r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
13 changes: 13 additions & 0 deletions programs/shl.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.text
.file "shl.zasm"
.globl __entry
__entry:
.func_begin0:
add 1, r0, r1
add 2, r0, r2
shl r1, r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
13 changes: 13 additions & 0 deletions programs/shl_conditional_eq.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.text
.file "shl_conditional_eq.zasm"
.globl __entry
__entry:
.func_begin0:
add 4, r0, r1
add 1, r0, r2
shl.eq r1, r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
16 changes: 16 additions & 0 deletions programs/shl_sets_eq_flag.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
.text
.file "shl_sets_eq_flag.zasm"
.globl __entry
__entry:

.func_begin0:
; EQ is set if the result is zero
add 0, r0, r1
add 0, r0, r2
shl! r1, r2, r1
sstore r0, r1
ret

.func_end0:
.note.GNU-stack
.rodata
17 changes: 17 additions & 0 deletions programs/shl_stack.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
.text
.file "shl_stack.zasm"
.globl __entry
__entry:
.func_begin0:
; grow stack
add 1, r0, stack+=[2]
; set stack value
add 4, r0, stack=[0]
add 2, r0, r2
; shift left
shl stack=[0], r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
13 changes: 13 additions & 0 deletions programs/shr.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.text
.file "shr.zasm"
.globl __entry
__entry:
.func_begin0:
add 8, r0, r1
add 2, r0, r2
shr r1, r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
13 changes: 13 additions & 0 deletions programs/shr_conditional_eq.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.text
.file "shr_conditional_eq.zasm"
.globl __entry
__entry:
.func_begin0:
add 8, r0, r1
add 2, r0, r2
shr.eq r1, r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
16 changes: 16 additions & 0 deletions programs/shr_sets_eq_flag.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
.text
.file "shr_sets_eq_flag.zasm"
.globl __entry
__entry:

.func_begin0:
; EQ is set if the result is zero
add 0, r0, r1
add 0, r0, r2
shr! r1, r2, r1
sstore r0, r1
ret

.func_end0:
.note.GNU-stack
.rodata
17 changes: 17 additions & 0 deletions programs/shr_stack.zasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
.text
.file "shr_stack.zasm"
.globl __entry
__entry:
.func_begin0:
; grow stack
add 1, r0, stack+=[2]
; set stack value
add 4, r0, stack=[0]
add 2, r0, r2
; shift right
shr stack=[0], r2, r3
sstore r0, r3
ret
.func_end0:
.note.GNU-stack
.rodata
3 changes: 2 additions & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use op_handlers::ptr_add::_ptr_add;
use op_handlers::ptr_pack::_ptr_pack;
use op_handlers::ptr_shrink::_ptr_shrink;
use op_handlers::ptr_sub::_ptr_sub;
use op_handlers::shift::_shift;
use op_handlers::sub::_sub;
pub use opcode::Opcode;
use state::VMState;
Expand Down Expand Up @@ -67,7 +68,7 @@ pub fn run_program_with_custom_state(bin_path: &str, mut vm: VMState) -> (U256,
Variant::Div(_) => _div(&mut vm, &opcode),
Variant::Jump(_) => todo!(),
Variant::Context(_) => todo!(),
Variant::Shift(_) => todo!(),
Variant::Shift(_) => _shift(&mut vm, &opcode),
Variant::Binop(_) => todo!(),
Variant::Ptr(ptr_variant) => match ptr_variant {
PtrOpcode::Add => _ptr_add(&mut vm, &opcode),
Expand Down
1 change: 1 addition & 0 deletions src/op_handlers/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ pub mod ptr_add;
pub mod ptr_pack;
pub mod ptr_shrink;
pub mod ptr_sub;
pub mod shift;
pub mod sub;
75 changes: 75 additions & 0 deletions src/op_handlers/shift.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
use crate::address_operands::{address_operands_read, address_operands_store};
use crate::value::TaggedValue;
use crate::{opcode::Opcode, state::VMState};
use zkevm_opcode_defs::Opcode as Variant;
use zkevm_opcode_defs::ShiftOpcode;

fn _shl(vm: &mut VMState, opcode: &Opcode) {
let (src0_t, src1_t) = address_operands_read(vm, opcode);
let (src0, src1) = (src0_t.value, src1_t.value);
let shift = (src1.low_u32() % 256) as usize;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not saying this is wrong per se, but I'm a bit confused.

The spec states that you take the lowest byte of the second input, here, you're taking the lowest 4 bytes, are we sure we aren't allowing something that shouldn't work based on the spec? We can also check how the fast vm is doing this to be completely sure.

Plus, I think the usize cast is redundant, why can't it simply be a u32?

Copy link
Contributor Author

@jorbush jorbush Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked fastVM and if we want something similar, we can use something like this instead:

let shift = src1.low_u32() as u8;

You are right that I get the last 4 bytes, but then I do the mod 256 and I get the last byte. I've done that so it's more aligned with the spec:

- The result is computed as in1 << (in2 % 256).

But I don't know the performance or readability of the code in this case, should I change it to the above alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the redundant usize, you were absolutely right, thanks!

Copy link
Contributor

@fkrause98 fkrause98 Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't know the performance or readability of the code in this case, should I change it to the above alternative?

As a rule of thumb, let's focus on simplicity+readability more than performance for now 🙂.

- The result is computed as in1 << (in2 % 256)
I'm a bit confused by the spec, it states you take the lowest byte as binary digits, so the max amount of shifts you can do
is 255, which is the range for numbers modulo 256. So the in2 % 256 looks a bit redundant.
But for now, I think following the spec as is is the best thing to do, so let's only take the second argument modulo 256,
and ignore the low 4 bytes, as it seems redundant.
Let's double check this with more people though, pinging @jrchatruc @IAvecilla @gianbelinche @juan518munoz.
Let's also have tests checking what happens when shifting with amounts greater than 256.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both src1.low_u32() and src1 % 256 would work.
I don't think the spec is redundant, since it asumes that in2 is a U256, so doing in2 % 256 is actually the way of getting the last byte

let res = src0 << shift;
if opcode.alters_vm_flags {
// Eq is set if result == 0
vm.flag_eq |= res.is_zero();
// other flags are reset
vm.flag_lt_of = false;
vm.flag_gt = false;
}
address_operands_store(vm, opcode, TaggedValue::new_raw_integer(res));
}

fn _shr(vm: &mut VMState, opcode: &Opcode) {
let (src0_t, src1_t) = address_operands_read(vm, opcode);
let (src0, src1) = (src0_t.value, src1_t.value);
let shift = (src1.low_u32() % 256) as usize;
let res = src0 >> shift;
jorbush marked this conversation as resolved.
Show resolved Hide resolved
if opcode.alters_vm_flags {
// Eq is set if result == 0
vm.flag_eq |= res.is_zero();
// other flags are reset
vm.flag_lt_of = false;
vm.flag_gt = false;
}
address_operands_store(vm, opcode, TaggedValue::new_raw_integer(res));
}

fn _rol(vm: &mut VMState, opcode: &Opcode) {
let (src0_t, src1_t) = address_operands_read(vm, opcode);
let (src0, src1) = (src0_t.value, src1_t.value);
let shift = (src1.low_u32() % 256) as usize;
let result = (src0 << shift) | (src0 >> (256 - shift));
fkrause98 marked this conversation as resolved.
Show resolved Hide resolved
if opcode.alters_vm_flags {
// Eq is set if result == 0
vm.flag_eq |= result.is_zero();
// other flags are reset
vm.flag_lt_of = false;
vm.flag_gt = false;
}
address_operands_store(vm, opcode, TaggedValue::new_raw_integer(result));
}

fn _ror(vm: &mut VMState, opcode: &Opcode) {
let (src0_t, src1_t) = address_operands_read(vm, opcode);
let (src0, src1) = (src0_t.value, src1_t.value);
let shift = (src1.low_u32() % 256) as usize;
let result = (src0 >> shift) | (src0 << (256 - shift));
if opcode.alters_vm_flags {
// Eq is set if result == 0
vm.flag_eq |= result.is_zero();
// other flags are reset
vm.flag_lt_of = false;
vm.flag_gt = false;
}
address_operands_store(vm, opcode, TaggedValue::new_raw_integer(result));
}

pub fn _shift(vm: &mut VMState, opcode: &Opcode) {
match opcode.variant {
jorbush marked this conversation as resolved.
Show resolved Hide resolved
Variant::Shift(ShiftOpcode::Shl) => _shl(vm, opcode),
Variant::Shift(ShiftOpcode::Shr) => _shr(vm, opcode),
Variant::Shift(ShiftOpcode::Rol) => _rol(vm, opcode),
Variant::Shift(ShiftOpcode::Ror) => _ror(vm, opcode),
_ => panic!("Unsupported shift operation"),
}
}
Loading