-
Notifications
You must be signed in to change notification settings - Fork 205
Ohadn/u128 encoded instr #1940
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
Ohadn/u128 encoded instr #1940
Conversation
9218ec3 to
a10d51d
Compare
|
|
Benchmark Results for unmodified programs 🚀
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1940 +/- ##
=======================================
Coverage 96.37% 96.37%
=======================================
Files 102 102
Lines 41238 41250 +12
=======================================
+ Hits 39742 39754 +12
Misses 1496 1496 ☔ View full report in Codecov by Sentry. |
a10d51d to
97b5922
Compare
97b5922 to
a10d51d
Compare
e7382da to
4f53823
Compare
1bec5ba to
afcbcea
Compare
4f53823 to
51f7f4d
Compare
afcbcea to
a77d76e
Compare
51f7f4d to
e4a3cb4
Compare
a77d76e to
6f060e8
Compare
1019002 to
23b291c
Compare
6f060e8 to
ee6ba48
Compare
ohad-nir-starkware
left a comment
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.
Reviewable status: 0 of 9 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
ee6ba48 to
b4d46f6
Compare
JulianGCalderon
left a comment
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.
LGTM
ohad-nir-starkware
left a comment
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.
@JulianGCalderon somehow reviewable marks you as both approving and blocking
Reviewable status: 0 of 9 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs
Outdated
| const OPCODE_EXTENSION_OFF: u64 = 63; | ||
| /// opcode_extension_num>0 is for new Stwo opcodes. | ||
| pub fn decode_instruction(encoded_instr: u128) -> Result<Instruction, VirtualMachineError> { | ||
| const HIGH_BITS: u128 = ((1 << 127) - (1 << 64)) << 1; |
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.
I would rename this variable to HIGH_BITS_MASK and document its purpose.
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.
Done.
vm/src/vm/decoding/decoder.rs
Outdated
| fn decode_offset(offset: u64) -> isize { | ||
| let vectorized_offset: [u8; 8] = offset.to_le_bytes(); | ||
| fn decode_offset(offset: u128) -> isize { | ||
| let vectorized_offset: [u8; 8] = (offset as u64).to_le_bytes(); |
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.
| let vectorized_offset: [u8; 8] = (offset as u64).to_le_bytes(); | |
| let vectorized_offset: [u8; 16] = offset.to_le_bytes(); |
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.
Done.
b4d46f6 to
43f4ba4
Compare
ohad-nir-starkware
left a comment
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.
Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs
Outdated
| const OPCODE_EXTENSION_OFF: u64 = 63; | ||
| /// opcode_extension_num>0 is for new Stwo opcodes. | ||
| pub fn decode_instruction(encoded_instr: u128) -> Result<Instruction, VirtualMachineError> { | ||
| const HIGH_BITS: u128 = ((1 << 127) - (1 << 64)) << 1; |
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.
Done.
vm/src/vm/decoding/decoder.rs
Outdated
| fn decode_offset(offset: u64) -> isize { | ||
| let vectorized_offset: [u8; 8] = offset.to_le_bytes(); | ||
| fn decode_offset(offset: u128) -> isize { | ||
| let vectorized_offset: [u8; 8] = (offset as u64).to_le_bytes(); |
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.
Done.
DavidLevitGurevich
left a comment
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.
Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions
a discussion (no related file):
this PR is irrelevant anymore right?
ohad-nir-starkware
left a comment
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.
Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions
a discussion (no related file):
Previously, DavidLevitGurevich wrote…
this PR is irrelevant anymore right?
It has been merged and then PR 1948 has been merged to remove NonZeroReservedBits.
u128 encoded instr
Description
Set the encoded instruction to be u128 for opcode_extensions to come.
Description of the pull request changes and motivation.
Checklist
This change is