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

Integer rings (Zq and instantiation for Labrador protocol) #769

Merged
merged 27 commits into from
Feb 20, 2025
Merged

Integer rings (Zq and instantiation for Labrador protocol) #769

merged 27 commits into from
Feb 20, 2025

Conversation

yshekel
Copy link
Collaborator

@yshekel yshekel commented Feb 11, 2025 •
edited
Loading

This PR introduces integer rings to ICICLE infrastructure.
NOTE: this PR is very large in terms of changed lines, but this it mostly code that moved due to sharing of code between finite-fields and integer-rings.

  • Zq type (Direct representation)
  • Vecops and NTT for Zq

Future PRs will add

  • ZqRns type (arithmetics in RNS representation - optimiztaion of Zq)
  • Conversions between Zq and ZqRns
  • Vecops and APIs for ZqRns
  • Rust and Go bindings
  • New APIs for rings (decomposition, matrix-multiplication and more) to support labrador and greyhound protocols)

cuda-backend-branch: yshekel/rings

@yshekel yshekel marked this pull request as ready for review February 16, 2025 15:59
@yshekel yshekel changed the title (Draft feature) Integer rings Integer rings (Zq and instantiation for Labrador protocol) Feb 16, 2025
mickeyasa
mickeyasa reviewed Feb 17, 2025
View reviewed changes
icicle/include/icicle/math/modular_arithmetic.h Outdated
friend HOST_DEVICE bool operator!=(const Derived& xs, const Derived& ys) { return !(xs == ys); }

template <typename Gen, bool IS_3B = false>
static HOST_DEVICE_INLINE Derived mul_weierstrass_b(const Derived& xs)
Copy link
Contributor

Choose a reason for hiding this comment

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

should move to field or change the name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it to projective class. It actually doesn't belong to field not ring. Agree?
I mean this weistrass-b is a curve parameter, has nothing to do with field type

mickeyasa
mickeyasa reviewed Feb 17, 2025
View reviewed changes
icicle/include/icicle/math/modular_arithmetic.h

static constexpr HOST_DEVICE_INLINE bool is_even(const Derived& xs) { return ~xs.limbs_storage.limbs[0] & 1; }

static constexpr HOST_DEVICE Derived inverse(const Derived& xs)
Copy link
Contributor

Choose a reason for hiding this comment

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

move to field.
In ring there should be a different functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will break code like NTT though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we agreed to keep it as is. Inverse returns 0 if no inverse exists

mickeyasa
mickeyasa reviewed Feb 17, 2025
View reviewed changes
icicle/include/icicle/rings/integer_ring.h
return hash;
}
};

Copy link
Contributor

Choose a reason for hiding this comment

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

where is the inverse?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

currently it's in the base class

mickeyasa
mickeyasa reviewed Feb 17, 2025
View reviewed changes
icicle/tests/test_ring_api.cpp Outdated

// Add ring specific tests here

// TYPED_TEST(RingAndFieldTest, /*Test Name*/) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

a test for the dedicated ring invers is missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's currently shared with field. I will have to split them yes

Copy link
Collaborator Author

@yshekel yshekel Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I split them

mickeyasa
mickeyasa reviewed Feb 17, 2025
View reviewed changes
icicle/tests/test_ring_field.h Outdated
ASSERT_EQ(0, memcmp(out_main.get(), out_ref.get(), batch_size * sizeof(int64_t)));
}

TEST_F(RingAndFieldTestBase, polynomialEval)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a meaning for that here? should move to field

Copy link
Collaborator Author

@yshekel yshekel Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It works and does not rely on inverse so I think let's keep it here.

mickeyasa
mickeyasa requested changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@mickeyasa mickeyasa left a comment

Choose a reason for hiding this comment

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

most of the comments are about the field-ring separation at the tests.
Also ring::inverse should have a different function - probably different signature

@yshekel
Copy link
Collaborator Author

yshekel commented Feb 17, 2025

most of the comments are about the field-ring separation at the tests. Also ring::inverse should have a different function - probably different signature

I am not sure it this is a good idea to make it different as it will break code such as NTT that relies on NTT.
In the design doc, we agreed to return 0 if no inverse exists. I did not do it yet.
BTW I think Tomer said that we may need polynomial division in the ring (it works for most cases in those rings we use).

@jeremyfelder jeremyfelder self-requested a review February 20, 2025 16:24
@yshekel yshekel merged commit c48b1bf into main Feb 20, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyfelder jeremyfelder jeremyfelder approved these changes

@mickeyasa mickeyasa mickeyasa approved these changes

@HadarIngonyama HadarIngonyama Awaiting requested review from HadarIngonyama

@LeonHibnik LeonHibnik Awaiting requested review from LeonHibnik

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

Successfully merging this pull request may close these issues.
3 participants
@yshekel @jeremyfelder @mickeyasa