min const generics #186

jon-chuang · 2021-01-22T05:06:55Z

Replacement for #182
Mostly done, what's left is to review/decide on API issues/integrate with rest of codebase (like curves)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (main)
Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
Wrote unit tests
Updated relevant documentation in the code
Added a relevant changelog entry to the Pending section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

Pratyush · 2021-01-22T07:38:48Z

ff/src/fields/arithmetic.rs

        }
    };
 }
-
-macro_rules! impl_field_square_in_place {
+// macro_rules! impl_deserialize_flags {


Suggested change

// macro_rules! impl_deserialize_flags {

Pratyush · 2021-01-22T07:39:32Z

ff/src/fields/arithmetic.rs

-                    }
-                    r[$limbs + i] = carry;
-                    carry = 0;
+// macro_rules! impl_deserialize_flags {


Suggested change

// macro_rules! impl_deserialize_flags {

Pratyush · 2021-01-27T22:29:55Z

The macro infrastructure is kinda difficult to follow; is there a way to simplify it? Also, I think merging the additive_ops and multiplicative_ops macro complicates the macro impl for only a small benefit. Could we revert that change?

Pratyush · 2021-01-27T22:36:24Z

ff/src/fields/models/fp.rs

+
+    #[inline]
+    fn two_adic_root_of_unity() -> Self {
+        Fp::<P, N>(P::TWO_ADIC_ROOT_OF_UNITY, PhantomData)


Suggested change

Fp::<P, N>(P::TWO_ADIC_ROOT_OF_UNITY, PhantomData)

Self::new(P::TWO_ADIC_ROOT_OF_UNITY)

Pratyush · 2021-01-27T22:37:01Z

ff/src/fields/models/fp.rs

+
+    #[inline]
+    fn large_subgroup_root_of_unity() -> Option<Self> {
+        Some(Fp::<P, N>(P::LARGE_SUBGROUP_ROOT_OF_UNITY?, PhantomData))


Suggested change

Some(Fp::<P, N>(P::LARGE_SUBGROUP_ROOT_OF_UNITY?, PhantomData))

P::LARGE_SUBGROUP_ROOT_OF_UNITY.map(Self::new)

Pratyush · 2021-01-27T22:37:38Z

ff/src/fields/models/fp.rs

+
+    #[inline]
+    fn multiplicative_generator() -> Self {
+        Fp::<P, N>(P::GENERATOR, PhantomData)


Suggested change

Fp::<P, N>(P::GENERATOR, PhantomData)

Self::new(P::GENERATOR)

Pratyush · 2021-01-27T22:39:34Z

ff/src/fields/models/fp.rs

+        if !self.is_zero() {
+            let mut tmp = P::MODULUS.clone();
+            tmp.sub_noborrow(&self.0);
+            Fp::<P, N>(tmp, PhantomData)


Suggested change

Fp::<P, N>(tmp, PhantomData)

Self::new(tmp)

jon-chuang · 2021-01-28T04:16:46Z

The macro infrastructure is kinda difficult to follow; is there a way to simplify it? Also, I think merging the additive_ops and multiplicative_ops macro complicates the macro impl for only a small benefit. Could we revert that change?

I've added some comments.

The main reason why this is annoying is that there is a lot of duplicate definitions. I think the real issue is that the macro impl is complicated. In hindsight it is true that the complication of making generic over concrete cases is unnecessary, so now I instantiate separately for concrete cases.

jon-chuang · 2021-01-28T05:42:43Z

To be quite honest, the limitations of const means that const generics at present buys little imo, under the hood, one is more or less instantiating concretely over limb types within ff anyway. So it's unclear what one should have expected or might hope is yet to be improved.

Pratyush · 2021-01-28T20:39:03Z

To be quite honest, the limitations of const means that const generics at present buys little imo, under the hood, one is more or less instantiating concretely over limb types within ff anyway. So it's unclear what one should have expected or might hope is yet to be improved.

I think the issue pops up because we can't have const-generic-based where clauses. Maybe for the time being this PR can move over just the BigInt stuff, and we can revisit Fp later?

jon-chuang · 2021-02-04T07:55:36Z

I would say that I prefer the previous method of just implementing concretely via macros. Using const generics while requiring literals for unrolls means some unpleasant workarounds that make it harder to modify the code.

huitseeker · 2021-03-26T17:21:19Z

I would consider adding a check for the minimal rustc version supporting `min_const_generics.

In fact, I have this in a commit you can grab here:
huitseeker@c69b1ed

Here's a sample output on an older compiler:
https://gist.github.com/cd63c76a4e53c2f4bbf19f9ca29f4eba

Pratyush · 2022-02-09T19:34:34Z

Superseded by #372 and #379

Resolves: arkworks-rs#186

@vlopes11

* feat: Add support for constraint usage within ark_bn254 This modifies the `Cargo.toml` with the following changes: - Add `ark-r1cs-std` as an optional dependency. - Add `ark-curve-constraint-tests` an a dev-dependency. - Add `r1cs` feature which activates `ark-r1cs-std` depencendy. (cherry picked from commit 48687352268d94a9dae74b50465273af15b0d132) * feat: Add `constraints` module for Bn245 Resolves: #186 * fix: Fix cherry-picking issues * chore: Conditionally activate `r1cs/std`. activate std for ark-r1cs-std only if r1cs is enabled as suggested by @vlopes11.

jon-chuang added 9 commits January 19, 2021 05:06

Preliminary commit for min const generics

dfd7c14

make NO_CARRY a const param now, instantiate fns for each # $limbs

295dc5f

instantiate only for desired limbs

00418a9

include unroll for into_repr

599bdeb

Unify impl_..._ops_from_ref to take const X: type and mult params

b568f31

massively reduce repetition in ops macro

a809a67

Fix some errors, cargo +nightly test passes.

52ee6db

contain in module

8421107

fix deserialize perf issues via vec! -> array

6612f4a

Pratyush reviewed Jan 22, 2021

View reviewed changes

Pratyush reviewed Jan 27, 2021

View reviewed changes

add comments

4a79d3f

jon-chuang added 3 commits January 28, 2021 13:04

Simplify macros

723da22

impl_additive

7dd28e3

minor changes

6b776ab

huitseeker mentioned this pull request Mar 28, 2021

Tiny-const-generics #255

Merged

6 tasks

daira mentioned this pull request Apr 16, 2021

Add support for 448-bit integers and fields #261

Merged

6 tasks

mmagician mentioned this pull request Dec 30, 2021

BigInt const generics #372

Merged

6 tasks

Pratyush closed this Feb 9, 2022

CPerezz added a commit to CPerezz/algebra that referenced this pull request Dec 20, 2023

feat: Add constraints module for Bn245

ceed4c1

Resolves: arkworks-rs#186

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

min const generics #186

min const generics #186

jon-chuang commented Jan 22, 2021

Pratyush Jan 22, 2021

Pratyush Jan 22, 2021

Pratyush commented Jan 27, 2021

Pratyush Jan 27, 2021

Pratyush Jan 27, 2021 •

edited

Loading

Pratyush Jan 27, 2021

Pratyush Jan 27, 2021

jon-chuang commented Jan 28, 2021 •

edited

Loading

jon-chuang commented Jan 28, 2021

Pratyush commented Jan 28, 2021

jon-chuang commented Feb 4, 2021 •

edited

Loading

huitseeker commented Mar 26, 2021

Pratyush commented Feb 9, 2022

	Fp::<P, N>(P::TWO_ADIC_ROOT_OF_UNITY, PhantomData)
	Self::new(P::TWO_ADIC_ROOT_OF_UNITY)

	Some(Fp::<P, N>(P::LARGE_SUBGROUP_ROOT_OF_UNITY?, PhantomData))
	P::LARGE_SUBGROUP_ROOT_OF_UNITY.map(Self::new)

	Fp::<P, N>(P::GENERATOR, PhantomData)
	Self::new(P::GENERATOR)

min const generics #186

min const generics #186

Conversation

jon-chuang commented Jan 22, 2021

Pratyush Jan 22, 2021

Choose a reason for hiding this comment

Pratyush Jan 22, 2021

Choose a reason for hiding this comment

Pratyush commented Jan 27, 2021

Pratyush Jan 27, 2021

Choose a reason for hiding this comment

Pratyush Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Pratyush Jan 27, 2021

Choose a reason for hiding this comment

Pratyush Jan 27, 2021

Choose a reason for hiding this comment

jon-chuang commented Jan 28, 2021 • edited Loading

jon-chuang commented Jan 28, 2021

Pratyush commented Jan 28, 2021

jon-chuang commented Feb 4, 2021 • edited Loading

huitseeker commented Mar 26, 2021

Pratyush commented Feb 9, 2022

Pratyush Jan 27, 2021 •

edited

Loading

jon-chuang commented Jan 28, 2021 •

edited

Loading

jon-chuang commented Feb 4, 2021 •

edited

Loading