-
Notifications
You must be signed in to change notification settings - Fork 0
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
RT: ALU: i8 multiplication #120
base: main
Are you sure you want to change the base?
Conversation
78eb482
to
7339b16
Compare
compiler-rt/src/alu/smul_with_overflow/smul_with_overflow_i8.cairo
Outdated
Show resolved
Hide resolved
69de2e1
to
d650038
Compare
c4370c6
to
96e7062
Compare
Is this ready for review / seems to be working, now? It looks that way, but I want to be sure given its history. (In the future, can we either use draft PRs or provide a "requires changes" self-review once we know something's not ready for merge, so we can avoid the confusing prior state this had?) |
@ktemkin it is ready and it was a draft all the time I was polishing the logic. I marked it as ready yesterday when I fixed the code. |
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! Only thing that might be nice to see is a design-level doc that explains the sign extension and why it is necessary, for those who come and look at this in the future and also might get caught up on it.
Implement the following polyfill: - `__llvm_mul_i8_u8` Signed-off-by: Wojciech Zmuda <[email protected]>
Implement the following polyfill: - `__llvm_umul_with_overflow_i8_i8` Signed-off-by: Wojciech Zmuda <[email protected]>
Implement the following polyfill: - `__llvm_smul_with_overflow_i8_i8` Signed-off-by: Wojciech Zmuda <[email protected]> Co-authored-by: Kate Temkin <[email protected]>
96e7062
to
97c27eb
Compare
Summary
Add the following polyfills:
Details
Checklist
scarb fmt
.