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

feat: added support for comparison operators #86

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kashbrti
Copy link
Contributor

@kashbrti kashbrti commented Dec 23, 2024

Description

added support for comparison operators between bignums

Problem*

Resolves

Summary*

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@kashbrti
Copy link
Contributor Author

We still need to add way more testing to this. also need to address the changes from the PR with the edge case bugs.

@kashbrti kashbrti marked this pull request as ready for review December 23, 2024 14:48
let carry_shift = 0x1000000000000000000000000000000;

let mut addend: [Field; N] = [0; N];
if comp_result == true {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was first thinking about skipping the if statement by casting the bool to Field and then computing intended_lhs = (1- comp_result as Field)* lhs + comp_result * rhs but not sure whether looping over the limbs and doing field multiplications would be any better than a conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

If you just want to swap the lhs and rhs then you can do

let (lhs, rhs) = if comp_result {
  (rhs, lhs)
} else {
  (lhs, rhs)
}

This would end up with needing to merge two [Field;N] values but that's more efficient than duplicating the logic currently in the if-statement. (This could be made better by just returning the element-wise difference between the two arrays rather than directly reordering them)

Copy link
Member

Choose a reason for hiding this comment

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

The best thing to do in this situation however would be to integrate the benchmarking system I added to https://github.com/noir-lang/noir-library-starter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks I was wondering if there's a way to do a swap in a single line :D

let underflow = b_u60.gte(a_u60);

if underflow {
temp_u60 = a_u60;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wonder how bad this is in brillig @TomAFrench. should I do what I did in the constrained version and just duplicate the code?

Copy link
Member

Choose a reason for hiding this comment

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

Could you not refactor this function to have a private implementation which accepts two U60Reprs?

pub(crate) unconstrained fn __cmp_remainder<let N: u32>(
    lhs: [Field; N],
    rhs: [Field; N],
) -> (bool, [Field; N], [bool; N], [bool; N]) {
    let mut a_u60: U60Repr<N, 2> = U60Repr::from(lhs);
    let mut b_u60: U60Repr<N, 2> = U60Repr::from(rhs);
    let mut temp_u60: U60Repr<N, 2> = U60Repr::one();
    let underflow = b_u60.gte(a_u60);

    if underflow {
        __cmp_remainder_priv(b_u60, a_u60);
    } else {
        __cmp_remainder_priv(a_u60, b_u60);
    }

Again, benchmarking would be way to make these decisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep I can make a private function that does the second part (computing carries etc). will do that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did the refactoring. I think we can refactor it a lot here. these operations are done over and over in all the with_flags functions.

@kashbrti kashbrti requested a review from TomAFrench December 23, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants