-
Notifications
You must be signed in to change notification settings - Fork 129
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
Make Expression copiable using singleton arena #354
base: main
Are you sure you want to change the base?
Conversation
ca5719e
to
c8c7ff9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
==========================================
- Coverage 82.09% 81.81% -0.29%
==========================================
Files 83 83
Lines 17228 17452 +224
==========================================
+ Hits 14143 14278 +135
- Misses 3085 3174 +89 ☔ View full report in Codecov by Sentry. |
c8c7ff9
to
09aa0b0
Compare
09aa0b0
to
8d1a0c7
Compare
…into experimental/nocopy1
use halo2_middleware::ff::Field; | ||
|
||
/// A field that allows to store `Expression`s in an arena. | ||
pub trait FieldFront: Field { |
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 believe this may introduce an issue in the use of generic fields:
A user may want to import a ff::Field
from a foreign library ( halo2curves for example), he won't be able to implement FieldFront
for this type and therefore, won't be able to plug it in the halo2-front.
Would it be possible to make a generic implementation so all Field
s automatically implement FieldFront
?
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.
Maybe, but there's currently another limitation in the current approach:
Arenas are field-especific and cannot be defined outside the halo2_crate ( this is due that is not possible to implement external traits on external types ).
So, users has to fork the library and add its own type inside.
It needs another approach if we want to be able to integrate foreign fields, yes.
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 there is no workaround this issue, this is a deal-breaker :/
NOTE: Relevant files to review are
halo2_frontend/src/plonk/circuit/{expession.rs, arena.rs}
This an experiment about making expression copiable. To implement this we go with this strategy:
ExprRef<F>
that contains the index (usize) of an expression stored in an specific arena.Expression
to useExprRef
instead the a recursiveExpression
that makes it non-copiable.FieldFront
. Mainly we change all frontend fromField
toFieldFront
, making arena accessible everywhere.This means:
<Field>
for<FieldFront>
and automatically all theirExpressions
are automatically copiable. This is thread-safe.