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.

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

fix(integer): remove double carry prop in sub #1469

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

tmontaigu
Copy link
Contributor

The subtraction is done via addition of the negation, the negation is done via unchecked_neg, this will make the first block have a carry.
Then we called add_assign_with_carry_parallelized which did a carry propagation on the rhs which here is the negated value, meaning the subtraction would do 2 carry propagation.

To fix that we directly call the lower function.

@cla-bot cla-bot bot added the cla-signed label Aug 12, 2024
The subtraction is done via addition of the negation,
the negation is done via unchecked_neg, this will make the
first block have a carry.
Then we called add_assign_with_carry_parallelized which did
a carry propagation on the rhs which here is the negated value,
meaning the subtraction would do 2 carry propagation.

To fix that we directly call the lower function.
@tmontaigu tmontaigu force-pushed the tm/fix-sub-double-carry-prop branch from cc4027a to 3284fad Compare August 12, 2024 16:19
@tmontaigu tmontaigu requested a review from IceTDrinker August 13, 2024 08:28
Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious how did the previous code do the propagation twice ?

Edit: nevermind missed the PR comment

@tmontaigu tmontaigu merged commit 594a5ce into main Aug 13, 2024
85 checks passed
@tmontaigu tmontaigu deleted the tm/fix-sub-double-carry-prop branch August 13, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants