-
Notifications
You must be signed in to change notification settings - Fork 234
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
Preliminary commit for min const generics #182
Conversation
ff/src/fields/arithmetic.rs
Outdated
#[inline] | ||
#[ark_ff_asm::unroll_for_loops] | ||
fn into_repr(&self) -> $BigIntegerType { | ||
// #[ark_ff_asm::unroll_for_loops] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you feel like the compiler can still optimize this well? Or should we try to do some manual unrolling inside the macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, for now, since we can no longer take advantage of literals in macro instantiations, I've decided one should have match arms and do the loop unroll for each of the possible num limbs.
This is extremely ugly, and I'm not sure if there's a workaround; the only plausible workaround for unrolling loops parametrised by const is proposal for compiler hints #[optimize(..)] that is not moving atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, basically, this is not the final form
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One can make the actual impl very compact, but under the hood, with macro expand etc it will be very ugly, just like our asm code selecting the num_limbs match arms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will simply modify the proc macro, exposing a new one that creates the match arms up to some limit, which can be a meta attr that can be parsed as usize for the match limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, for a specific problem, which is copying the data back over in each of the match arms, since there would be a type mismatch, it might make sense to do an unsafe transmute of a pointer; however, this would violate our desire to have safe code.
Amazing, thanks for this!
Maybe we can use techniques from the C++ world for this? Eg: https://www.informit.com/articles/article.aspx?p=30667&seqNum=7 It could get complicated though.
Hmm that's unfortunate; it's due to this issue: rust-lang/rust#43408
Can't the breaking changes be worked around by still exporting |
ff/src/fields/models/fp.rs
Outdated
}; | ||
use ark_serialize::*; | ||
|
||
pub trait FpParams<const N: usize>: FpParameters<BigInt = BigInt<N>> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get rid of this trait now, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, for now, I use it to instantiate NO_CARRY as a const param. Since otherwise, it was troublesome to ensure it would evaluate at compile time.
Well for this, Im not sure if it will work; notice the metaprogramming examples use literals as the param. I think something a lot of people would like is the ability to do metaprogramming based on const. Unfortunately const is something evaluated at MIR level, so there is no longer any syntax to do metaprogramming with. Still, it would be really cool to have functionality to do metaprogramming at MIR level.
We could. But, one wonders if it is better to make the breaking change; personally I'm torn about this. |
d30db1d
to
db1878a
Compare
84ccc09
to
1590ee0
Compare
So I would say at this point that Im quite happy with state of PR, except for having checked for performance regressions and careful review. Also the number of unresolved questions about API and so on. But the core of the code is looking pretty good if you ask me. |
Here is some preliminary data for the performance regression tests. There appears to be some problems with the serialization, and more pertinently, the Apart from that, things seem alright. So I discovered with serialize/deserialize, and discovered that using vec! instead of array is causing the slowdown, now it is fixed. So perf wise, I would say there are no issues. |
Description
To do:
[T; N * 2]
where N is generic being impossibleTests should pass on nightly 1.51.
Thoughts:
limitations still exist for min const generics, especially for our literal-dependent loop unrolling use case, and also const eval with generics.Currently, this has been worked aroundBigInt::<4>([0, 0, ..])
Update:
TODO:
Thoughts:
mul_without_reduce
is instantiated using loops that are not forced to be unrolled, so it is unclear what the compilers behaviour will be. It is only used in runtime code whenNO_CARRY
is false however. So I think its not a big issue. Still.Currently, this is how mul_assign looks macro expanded:
So, rather neat. But notice we define functions for each n limbs. Since we can control exactly which numbers we instantiate for via the
limb_instantiation!(1, 4, 5, 6, 12, 13);
macro, I think unsavoury code bloat is substantially reduced.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