-
Notifications
You must be signed in to change notification settings - Fork 161
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
Implement SHA256 in Miden Assembly #123
Conversation
itzmeanjan
commented
Feb 22, 2022
•
edited
Loading
edited
- SHA256 functions; see here
- SHA256 message schedule preparation; see here
- SHA256 digest compute; see here
Current cost of executing SHA256 related functions on Miden VM
|
VM Cycles required to compute SHA256 2-to-1 hash: 29861 |
See itzmeanjan#1, where I improve SHA256 implementation |
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.
Looks great! I left a small comment and also highlighted a few minor optimization opportunities.
I think we can optimize this a bit more, but this can be done in the future PRs.
popw.mem # write to mem msg[24, 25, 26, 27] # | ||
pushw.local.4 |
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.
This could be replaced by something like:
storew.mem
loadw.local.4
This change should save 8 cycles.
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.
Noted, will revisit sha256 soon.
popw.mem # write to mem msg[32, 33, 34, 35] # | ||
pushw.local.4 |
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.
This could be replaced by something like:
storew.mem
loadw.local.4
This change should save 8 cycles.
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.
Noted
popw.mem # write to mem msg[40, 41, 42, 43] # | ||
pushw.local.4 |
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.
This could be replaced by something like:
storew.mem
loadw.local.4
This change should save 8 cycles.
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.
Noted
let inputs = ProgramInputs::new(&i_words, &[], Vec::new()).unwrap(); | ||
let trace = execute(&script, &inputs).unwrap(); | ||
let last_state = trace.last_stack_state(); |
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.
You might be able to use the new build_test
macro from here to simplify this a bit.
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.
Thanks, changed here d46ae2e
popw.mem # write to mem msg[36, 37, 38, 39] # | ||
pushw.local.4 |
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.
This could be replaced by something like:
storew.mem
loadw.local.4
This change should save 8 cycles.
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.
Noted
popw.mem # write to mem msg[44, 45, 46, 47] # | ||
pushw.local.4 |
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.
This could be replaced by something like:
storew.mem
loadw.local.4
This change should save 8 cycles.
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.
Noted
…nning using newly defined macro
Thanks for reviewing it @bobbinth ! I'm keeping a note of optimization suggestions you've shared, in next few weeks I'll revisit both blake3 & sha256. Also I've moved both blake3 & sha256 tests to use new |
399d1fa
to
5c93bef
Compare
c7f42e3
to
fa33733
Compare