Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor on mul/mul_add gadget #296

Merged
merged 7 commits into from
Oct 3, 2024
Merged

Refactor on mul/mul_add gadget #296

merged 7 commits into from
Oct 3, 2024

Conversation

hero78119
Copy link
Collaborator

@hero78119 hero78119 commented Sep 30, 2024 •
edited
Loading

Previously attempt PR #291

This PR cover

  • process lo limb overflow correctly on mul/mul_add gadget and constrain carry range via LtGadget
    1. hi limb will be handled in next PR to address "mulhu" opcode implementation
    2. constrain via LtGadget save lot of cell comparing with bit decomposition
  • pass u16 native value in assign function and convert to base field in last second

    this help to clean up many redundant conversion in codebase

  • correctly handling unittest in arithmetics.rs via MockProver. Previously with expression evaluation can't capture any error
  • refactor and rename few function/gadget

"mulhu opcode" #130 will be build based on this PR

@hero78119 hero78119 marked this pull request as draft September 30, 2024 09:44
@hero78119 hero78119 changed the title fix mul uint arithmetics to support lo/hi limb MULUH implementation and uint mul bug fix Sep 30, 2024
@hero78119 hero78119 changed the title MULUH implementation and uint mul bug fix Refactor on mul/mul_add gadget Oct 2, 2024
@hero78119 hero78119 marked this pull request as ready for review October 2, 2024 12:46
chaosma
chaosma reviewed Oct 3, 2024
View reviewed changes
ceno_zkvm/src/uint/util.rs Outdated Show resolved Hide resolved
ceno_zkvm/src/uint.rs Outdated Show resolved Hide resolved
ceno_zkvm/src/uint.rs Outdated Show resolved Hide resolved
ceno_zkvm/src/uint.rs Show resolved Hide resolved
@hero78119 hero78119 force-pushed the bug/fix_mul_impl branch 2 times, most recently from bd69824 to 9a61129 Compare October 3, 2024 10:01
@hero78119 hero78119 enabled auto-merge (squash) October 3, 2024 15:05
@hero78119 hero78119 merged commit 25c9f2e into master Oct 3, 2024
6 checks passed
@hero78119 hero78119 deleted the bug/fix_mul_impl branch October 3, 2024 15:15
@hero78119 hero78119 mentioned this pull request Oct 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chaosma chaosma chaosma approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hero78119 @chaosma