-
Notifications
You must be signed in to change notification settings - Fork 211
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
feat(jolt-core): Adding new multiplication instructions #303
Conversation
5e8cfb1
to
baa2a66
Compare
59332f5
to
e1d0836
Compare
Hey @moodlezoup this PR should be ready for review now. I originally planned to include the MULHU instruction in this as well, but was failing at implementing it (skill issue). I based the MUL, and MULU instructions closely off what we have for the ADD and SUB instructions, would love to know if this approach makes sense or if I should modify to have a custom sub-table for this. |
f8c998e
to
0c3d7ba
Compare
Appreciate the direction setting on this in DMs moodlezoup, got the MULHU instruction to work :) |
This will allow us to add the first 3 instructions of the RISC V M extension, MUL, MULU and MULHU.
0c3d7ba
to
42e1da4
Compare
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 for working on this! One request for the MULHU instruction
let msb_chunk_index = C - (WORD_SIZE / log2(M) as usize) - 1; | ||
// Reversed the order of the subtable indices compared to MUL and MULU | ||
vec![ | ||
( | ||
Box::new(TruncateOverflowSubtable::<F, WORD_SIZE>::new()), | ||
SubtableIndices::from(msb_chunk_index + 1..C), | ||
), | ||
( | ||
Box::new(IdentitySubtable::new()), | ||
SubtableIndices::from(0..msb_chunk_index + 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.
For our current parameters C = 4
and M = 2^16
, the TruncateOverflowSubtable
here is just all zeros (which is what we want), but I think this would be broken for C
and M
where C * log2(M) != 2 * WORD_SIZE
For now, let's just add an assert_eq!
and remove the TruncateOverflowSubtable
let msb_chunk_index = C - (WORD_SIZE / log2(M) as usize) - 1; | |
// Reversed the order of the subtable indices compared to MUL and MULU | |
vec![ | |
( | |
Box::new(TruncateOverflowSubtable::<F, WORD_SIZE>::new()), | |
SubtableIndices::from(msb_chunk_index + 1..C), | |
), | |
( | |
Box::new(IdentitySubtable::new()), | |
SubtableIndices::from(0..msb_chunk_index + 1), | |
), | |
] | |
assert_eq!(C * log2(M), 2 * WORD_SIZE); | |
vec![ | |
( | |
Box::new(IdentitySubtable::new()), | |
SubtableIndices::from(0..C / 2), | |
), | |
] |
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.
(there are existing instructions that assume C = 4 and M = 2^16, so I think this is fine for now)
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.
Sounds good, went with your approach. I also had to change the concatenate looks param to take in vals.len() instead of C for it to not panic.
459b97f
to
40bc2aa
Compare
…zoup We refactor to remove the TruncateOverflowSubtable and chaned the params in the combine lookups function.
40bc2aa
to
3a7f86c
Compare
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.
nice work!
Jolt will now support some multiplication instructions - MUL, MULU for the RISC-V M extension.
Addresses parts of #251