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

[Refactoring] Avoid using FieldElements until creating ACIR #449

Open
jfecher opened this issue Nov 7, 2022 · 4 comments
Open

[Refactoring] Avoid using FieldElements until creating ACIR #449

jfecher opened this issue Nov 7, 2022 · 4 comments
Assignees
Labels

Comments

@jfecher
Copy link
Contributor

jfecher commented Nov 7, 2022

Problem

Using FieldElements to represent all integers during compile-time makes it more cumbersome and difficult to perform operations on them when they are known constants. Examples of this can be seen in the SSA pass currently: constant conversions back and forth between FieldElement, u128, and occasionally BigUInt. It is easy to forget to check the bitcount before performing FieldElement::to_u128 leading to a possible runtime error as well (e.g. on line 421 of node.rs).

Solution

Instead, we can represent all integers as arbitrarily-sized BigInts- possibly with the exception of the sized integer types which could be i128 to better perform bitwise operations.

@jfecher jfecher added the enhancement New feature or request label Nov 7, 2022
@jfecher jfecher self-assigned this Nov 7, 2022
@kevaundray
Copy link
Contributor

Where would you store the field modulus?

CC @guipublic as this concerns the SSA pass

@guipublic
Copy link
Contributor

Avoid using fields until ACIR is not the right thing to do. As field is a (major) noir type, we would need to do constant folding with fields in the ssa pass.
Replacing fields with bigints is also not a good idea. I remember using them at some point (for overflow checks I believe) and I got a massive performance hit. BigInts are unlimited in theory, but in practice doing operation with big values can become very slow very quickly, so they must be used with caution as well.

@jfecher
Copy link
Contributor Author

jfecher commented Nov 16, 2022

I believe BigInts specifically are also not the right abstraction as we would not be able to perform bitwise operations on them. Perhaps keeping FieldElements for field types and representing integer types another way (i128 or u128?) would be more accurate.

@kevaundray
Copy link
Contributor

Maybe orthogonal; in the future, it would be good to store multiple integers in a single field. If we have 2 u64s, the compiler should be able to pack that into a Field

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants