Mul and div opcodes #26

juan518munoz · 2024-06-13T19:57:59Z

closes #3

chore: split program files into associated opcode dir for clarity

fkrause98 · 2024-06-13T20:10:02Z

Makefile

-ASM_PROGRAMS = $(wildcard $(PROGRAMS_DIR)/*.zasm)
-ARTIFACTS_YUL = $(patsubst $(PROGRAMS_DIR)/%.yul, $(ARTIFACTS_DIR)/%.artifacts.yul, $(YUL_PROGRAMS))
-ARTIFACTS_ASM = $(patsubst $(PROGRAMS_DIR)/%.zasm, $(ARTIFACTS_DIR)/%.artifacts.zasm, $(ASM_PROGRAMS))
+YUL_PROGRAMS := $(shell find $(PROGRAMS_DIR) -type f -name "*.yul")


Why was this change needed?

Not really needed, it's only to avoid eventually having to many similarly named tests on the same directory

Makes sense to me, let's just be 100% sure that it does not bring any unwanted problems when running tests and such.

fkrause98 · 2024-06-13T20:23:54Z

src/op_handlers/mul.rs

+
+pub fn _mul(vm: &mut VMState, opcode: Opcode) {
+    let (src0, src1) = address_operands_read(vm, &opcode);
+    let res = src0 * src1;


These are 256 bit numbers, keep in mind the spec specifies the mul instruction multiplies the two numbers modulo 2**512.
The high and low bits are also returned in 2 separate operands, I think address_operands_read only takes into account the first operand destination, correct me If I'm wrong.
Also, we should check if overflow has occurred with mul_overflow, check #25 for an example on how to do this.

Ideally, this PR should also implement the logic for the flag changes, if specified. You can also check #25 for an example.

You are right, I'll change the function, it's corresponding test and check overflow.

Should we also implement the logic for flag changes in this PR, or leave it for a separate one as it's been done with #25?

Yes, implement the logic flag here, and bring the changes from #25 now that is merged.

fkrause98 · 2024-06-13T20:25:45Z

tests/integration_test.rs

+    assert_eq!(remainder_result, U256::from_dec_str("1").unwrap());
+}
+
+#[test]


Good job with these tests.

gianbelinche · 2024-06-13T20:58:29Z

src/op_handlers/div.rs

+    vm.set_register(opcode.dst0_index, src0 / src1);
+    vm.set_register(opcode.dst1_index, src0 % src1);


Here you should use address_operands_store for storing the results, right now it only cares about the dst0, it shouldn't be much different to implement it for dst1

Right, the logic of both mul and div for address_operand_store need to be implemented then.

gianbelinche · 2024-06-13T21:00:05Z

programs/div/div_quotient.zasm

Eventually this will need to be tested with different input formats, like stack and codepage

gianbelinche

Good job with the fixes!

gianbelinche · 2024-06-18T15:11:22Z

src/address_operands.rs

 }

-pub fn address_operands_store(vm: &mut VMState, opcode: &Opcode, res: U256) {
+pub fn address_operands_store(vm: &mut VMState, opcode: &Opcode, res: (U256, Option<U256>)) {


Instead of passing an Option, wouldn't it be best to just have 2 functions, one for dst0 and the other for dst1, since the implementation is different.
I think this adds noise

Since this is a very special case, I think it's worth adding another implementation for div and mul, here's my proposal:

pub fn address_operands_store(vm: &mut VMState, opcode: &Opcode, res: U256) { address_operands(vm, opcode, (res, None)) } pub fn address_operands_div_mul(vm: &mut VMState, opcode: &Opcode, res: (U256, U256){ address_operands(vm, opcode, (res, Some(U256)) } fn address_operands(vm: &mut VMState, opcode: &Opcode, res: (U256, Option<U256>)) { ... }

gianbelinche · 2024-06-18T15:14:12Z

src/op_handlers/mul.rs

+            low_bits.try_into().unwrap(),
+            Some(high_bits.try_into().unwrap()),


Maybe add a comment saying that since we used the u256_mask we already know this unwraps won't fail

fkrause98

Almost there, I think what's more important here is the function addressing, I think my proposal is a nice way to solve this, but feel free to come up with something else if you want. The only thing I'm more worried about it's that the address operands store signature function should be as clean as possible.

fkrause98 · 2024-06-18T19:44:45Z

src/address_operands.rs

 }

-pub fn address_operands_store(vm: &mut VMState, opcode: &Opcode, res: U256) {
+pub fn address_operands_store(vm: &mut VMState, opcode: &Opcode, res: (U256, Option<U256>)) {


Since this is a very special case, I think it's worth adding another implementation for div and mul, here's my proposal:

pub fn address_operands_store(vm: &mut VMState, opcode: &Opcode, res: U256) { address_operands(vm, opcode, (res, None)) } pub fn address_operands_div_mul(vm: &mut VMState, opcode: &Opcode, res: (U256, U256){ address_operands(vm, opcode, (res, Some(U256)) } fn address_operands(vm: &mut VMState, opcode: &Opcode, res: (U256, Option<U256>)) { ... }

src/op_handlers/mul.rs

fkrause98 · 2024-06-18T19:48:08Z

tests/integration_test.rs

+}
+
+#[test]
+fn test_more_complex_program_with_conditionals() {


Great job thoroughly testing this, must have taken some time 🚀

juan518munoz added 5 commits June 13, 2024 11:13

impl mul

9f7a812

add div

0f03c65

merge branch 'main'

9c84aa9

refactor makefile and programs dir

fa70cc6

add missing flag for test

56bf594

fkrause98 requested changes Jun 13, 2024

View reviewed changes

gianbelinche reviewed Jun 13, 2024

View reviewed changes

juan518munoz added 9 commits June 13, 2024 18:02

merge branch 'main'

6a463bd

add flag setting for mul

e39b8f5

fix mul not following spec

7ffeace

impl div set flags

30d6ed2

div set flags test

e671111

more tests

acdfe78

clippy

838ec36

add mul conditional test

02ca9a9

add div conditional tests

552140f

gianbelinche approved these changes Jun 18, 2024

View reviewed changes

juan518munoz requested a review from fkrause98 June 18, 2024 18:20

fkrause98 requested changes Jun 18, 2024

View reviewed changes

juan518munoz added 4 commits June 18, 2024 17:34

wrap address_operands

a5f9d4e

justify unwrap

d328822

Merge branch 'main' into mul-and-div-opcodes

d481767

remove makefile changes

31f251c

fkrause98 approved these changes Jun 19, 2024

View reviewed changes

fkrause98 merged commit de1192b into main Jun 19, 2024
3 checks passed

fkrause98 deleted the mul-and-div-opcodes branch June 19, 2024 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mul and div opcodes #26

Mul and div opcodes #26

juan518munoz commented Jun 13, 2024

fkrause98 Jun 13, 2024

juan518munoz Jun 13, 2024

fkrause98 Jun 13, 2024

fkrause98 Jun 13, 2024

juan518munoz Jun 13, 2024

fkrause98 Jun 13, 2024 •

edited

Loading

fkrause98 Jun 13, 2024

gianbelinche Jun 13, 2024

juan518munoz Jun 13, 2024

gianbelinche Jun 13, 2024

gianbelinche left a comment

gianbelinche Jun 18, 2024

fkrause98 Jun 18, 2024

gianbelinche Jun 18, 2024

fkrause98 left a comment

fkrause98 Jun 18, 2024

fkrause98 Jun 18, 2024

		vm.set_register(opcode.dst0_index, src0 / src1);
		vm.set_register(opcode.dst1_index, src0 % src1);

		low_bits.try_into().unwrap(),
		Some(high_bits.try_into().unwrap()),

Mul and div opcodes #26

Mul and div opcodes #26

Conversation

juan518munoz commented Jun 13, 2024

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

fkrause98 Jun 13, 2024 • 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

gianbelinche 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

fkrause98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fkrause98 Jun 13, 2024 •

edited

Loading