-
Notifications
You must be signed in to change notification settings - Fork 248
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
Generalised version of improved fp2 #212
Conversation
I think to really generalise this, we need to define some way to define multiplications with small integers for fields. Or, we should have some way to indicate that a field element is really small. This should be some compile-time data, to indicate that a particular field element is a small constant. However, the threshold for how small would be considered small would depend on the number of limbs in the field. Further, this only makes sense for constant data. So this is sad. However, we can use const generics to achieve this... Nonetheless, we actually don't need so much generality, so I will investigate tweaking the concrete FpN impls to use more efficient autogenerated versions of mul by residue if we make them available. |
123f692
to
4f711ab
Compare
c35c023
to
3163912
Compare
What about adding a pub trait Field: Mul<<Self as Field>::SmallValue, Output = Self> {
...
type SmallValue: Neg + Mul<Self, Output = Self> + Into<Self>;
} We can use it as follows: // Impl for prime fields FpN:
impl Field for FpN {
type Smallvalue = i8;
}
// Impl for quadratic extension fields:
impl Field for QuadExtField {
type Smallvalue = Pair<Self::BaseField>;
}
pub trait QuadExtParameters {
const NONRESIDUE: Pair<Self::BaseField>; // No need for separate NONRESIDUE_SMALL
// No need for mul_by_nonresidue, because it's subsumed by the trait bounds above
}
pub struct Pair<F: Field> {
// The coordinates can be either small or full-sized.
c0: Either<F, F::SmallValue>,
c1: Either<F, F::SmallValue>,
} (Similarly for cubic ext field) |
Hmm, that was something that crossed my mind, but I wanted something that could be dropped in so that one wouldn't have to rewrite any formulas. However, this seemed too complicated. Do you think your idea might have any advantages over what this PR does? Hence, I just resorted in the end to defining a specific method, Do you know if there is any methods apart from nonresidue muls where |
I should probably write some tests for |
The benefit of the foregoing approach is primarily that it enables multiplication by arbitrary small field elements cheaply. For example, one could use it also to replace the It would also reduce some complexity in writing new curves and fields. For example, right now, when implementing a new extension field, one could forget to specialize the We could also use it for #209 Overall it would allow us to remove many unseemly hacks from the codebase =) |
Hmm, but do we have the same idea of what mul_by_small_element entails? There are two possibilities essentially:
In my mind, it makes sense to define these as two separate functionalities, and one can already create these functions generically in the PrimeField trait or Fp struct, for instance. |
Why not have a threshold? For small enough numbers, we use custom addition chains, and then after that we use the specialized montgomery mul (which we can add at a later date) |
Well, determining that threshold is probably not that straightforward. I would rather let the downstream user decide which method they'd like to deploy, or if they'd like to set a threshold. Btw, regarding mul_by_a, I noticed that
results in non-trivial assembly for f. This means that rust probably can't optimise the zero additions away, sadly. Hence I will also submit a PR to rewrite some formulas in adding results of mul_by_a, when a is zero. OK, short weierstrass already does this for the addition formula. |
For the case of PrimeFields we can support that use case by also requiring EDIT: Actually you don't even need separate cases, you just defined pub enum SmallInt {
UseAdditionChain(i8),
UseSpecialMontMul(i64),
} This way the choice is left to the user. |
Turns out I made a mistake in implementation, by default, I used naive mul_assign with Self::Residue rather than mul_base_field_by_residue. Now, it should be back to before for tower fields. |
Related to #207
The main contribution of this PR is to get rid of multiple redundant functions, and to unify all mul by small const methods into default implementations.
Results vis a vis pre #207:
Often, we get speedups for free (for instance for bw6). I'm still investigating why this is.
mnt4_298's residue is G2 basefield non-residue is 17, so it's not worthwhile.
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.
Pending
section inCHANGELOG.md
Files changed
in the Github PR explorer