introduce opcode_extension to the structure of Instruction. #1933
Conversation
ohad-nir-starkware
requested review from
igaray,
Oppen,
fmoletta,
juanbono,
pefontana and
gabrielbosio
as code owners
ohad-nir-starkware
force-pushed
the
ohadn/opcode_extension
branch
from
e7c4eb6 to
6dbac44
Compare
|
|
Benchmark Results for unmodified programs 🚀
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1933 +/- ##
=======================================
Coverage 96.35% 96.36%
=======================================
Files 102 102
Lines 41095 41173 +78
=======================================
+ Hits 39599 39677 +78
Misses 1496 1496 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed 4 of 6 files at r1, all commit messages.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @ohad-nir-starkware, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs line 9 at r1 (raw file):
// opcode_extension| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg // 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0
I suggest ... 15 and then it is clear that it's all the evelent.
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @ohad-nir-starkware, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs line 9 at r1 (raw file):
Previously, DavidLevitGurevich wrote…
I suggest
... 15and then it is clear that it's all the evelent.
- all the elements to the left
ohad-nir-starkware
force-pushed
the
ohadn/opcode_extension
branch
from
6dbac44 to
e15c6e6
Compare
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs line 9 at r1 (raw file):
Previously, DavidLevitGurevich wrote…
- all the elements to the left
Done.
There was a problem hiding this comment.
Reviewed 2 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewed 2 of 6 files at r1, all commit messages.
Reviewable status:
vm/src/vm/decoding/decoder.rs
Outdated
| // Grab offsets and convert them from little endian format. | ||
| let off0 = decode_offset(encoded_instr >> OFF0_OFF & OFFX_MASK); | ||
| let off1 = decode_offset(encoded_instr >> OFF1_OFF & OFFX_MASK); | ||
| let off2 = decode_offset(encoded_instr >> OFF2_OFF & OFFX_MASK); | ||
|
|
||
| // Grab flags | ||
| let flags = encoded_instr >> FLAGS_OFFSET; | ||
| let flags = (encoded_instr >> FLAGS_OFFSET) & FLAGS_MASK; |
There was a problem hiding this comment.
As FLAGS_MASK == 0x7FFF, we would be ignoring the highest bit and allowing it to be 1, ignoring the check at lines 101-109, right? Is this the required behaviour?
There was a problem hiding this comment.
I think so.
@Stavbe is that right?
There was a problem hiding this comment.
Aah, my bad. It seems that the variable used for the checks 101-109 is encoded_instr and not flags, so this isn't an issue.
There was a problem hiding this comment.
The flags variable is only used to grab individual flags in the next lines (45-51). Bitmasking flags with FLAGS_MASK adds an extra operation that does not introduce a logical change in the code. While this is not a big decrement in code readability, I suggest to keep this PR as small as possible by removing the change in this line and line 31.
ohad-nir-starkware
force-pushed
the
ohadn/opcode_extension
branch
from
e15c6e6 to
1d1be8b
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @Stavbe, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs
Outdated
| // Grab offsets and convert them from little endian format. | ||
| let off0 = decode_offset(encoded_instr >> OFF0_OFF & OFFX_MASK); | ||
| let off1 = decode_offset(encoded_instr >> OFF1_OFF & OFFX_MASK); | ||
| let off2 = decode_offset(encoded_instr >> OFF2_OFF & OFFX_MASK); | ||
|
|
||
| // Grab flags | ||
| let flags = encoded_instr >> FLAGS_OFFSET; | ||
| let flags = (encoded_instr >> FLAGS_OFFSET) & FLAGS_MASK; |
There was a problem hiding this comment.
I think so.
@Stavbe is that right?
There was a problem hiding this comment.
Reviewed 1 of 6 files at r1, all commit messages.
Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @Stavbe)
vm/src/vm/decoding/decoder.rs line 11 at r3 (raw file):
// ... 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 /// Decodes an instruction. The encoding is little endian, so flags go from bit 63 to 48.
Expand a bit upon the bits reserved for opcode_extension.
Code quote:
Decodes an instruction. The encoding is little endian, so flags go from bit 63 to 48.vm/src/vm/decoding/decoder.rs line 106 at r3 (raw file):
return Err(VirtualMachineError::InvalidOpcodeExtension( opcode_extension_num, ))
Please add a test that covers decoding an instruction that returns the new vm error variant.
Code quote:
return Err(VirtualMachineError::InvalidOpcodeExtension(
opcode_extension_num,
))There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, 2 of 2 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @ohad-nir-starkware, @Oppen, @pefontana, and @Stavbe)
vm/src/vm/decoding/decoder.rs
Outdated
| // Grab offsets and convert them from little endian format. | ||
| let off0 = decode_offset(encoded_instr >> OFF0_OFF & OFFX_MASK); | ||
| let off1 = decode_offset(encoded_instr >> OFF1_OFF & OFFX_MASK); | ||
| let off2 = decode_offset(encoded_instr >> OFF2_OFF & OFFX_MASK); | ||
|
|
||
| // Grab flags | ||
| let flags = encoded_instr >> FLAGS_OFFSET; | ||
| let flags = (encoded_instr >> FLAGS_OFFSET) & FLAGS_MASK; |
There was a problem hiding this comment.
The flags variable is only used to grab individual flags in the next lines (45-51). Bitmasking flags with FLAGS_MASK adds an extra operation that does not introduce a logical change in the code. While this is not a big decrement in code readability, I suggest to keep this PR as small as possible by removing the change in this line and line 31.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fmoletta, @igaray, @juanbono, @JulianGCalderon, @ohad-nir-starkware, @Oppen, @pefontana, and @Stavbe)
ohad-nir-starkware
force-pushed
the
ohadn/opcode_extension
branch
from
1d1be8b to
dfd3f66
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fmoletta, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @Stavbe, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs line 11 at r3 (raw file):
Previously, YairVaknin-starkware wrote…
Expand a bit upon the bits reserved for opcode_extension.
Done.
vm/src/vm/decoding/decoder.rs line 106 at r3 (raw file):
Previously, YairVaknin-starkware wrote…
Please add a test that covers decoding an instruction that returns the new vm error variant.
Done.
vm/src/vm/decoding/decoder.rs
Outdated
| // Grab offsets and convert them from little endian format. | ||
| let off0 = decode_offset(encoded_instr >> OFF0_OFF & OFFX_MASK); | ||
| let off1 = decode_offset(encoded_instr >> OFF1_OFF & OFFX_MASK); | ||
| let off2 = decode_offset(encoded_instr >> OFF2_OFF & OFFX_MASK); | ||
|
|
||
| // Grab flags | ||
| let flags = encoded_instr >> FLAGS_OFFSET; | ||
| let flags = (encoded_instr >> FLAGS_OFFSET) & FLAGS_MASK; |
Introduce opcode_extension to the structure of Instruction
Description
In preparation for adding new opcodes to Stwo, we introduce another field named opcode_extension to the structure Instruction.
That field is an enum that in the future will denote which of the new opcodes is being used in the instruction.
Checklist
This change is