Change gate evaluation memory layout #390
Conversation
nbgl
changed the title
|
Sorry for the delay, will try to review tomorrow |
| fn eval_unfiltered_base_one( | ||
| &self, | ||
| vars: EvaluationVarsBase<F>, | ||
| mut yield_constr: StridedConstraintConsumer<F>, |
There was a problem hiding this comment.
nit: it seems a little unusual to me for an object name to be a verb, maybe call it something like a constraint buffer/consumer?
There was a problem hiding this comment.
(Not important though, feel free to disregard for now if it'd create conflicts)
There was a problem hiding this comment.
This is a case where I thought clarity justified breaking convention. I was going for the feel of Python's generator syntax (yield statement).
yield_constr would ideally be a closure, except I wanted to add a many method for convenience when working with iterators. I can't make StridedConstraintConsumer callable without #![feature(fn_traits)], but functionally it's closer to a closure than to a struct: we don't care about the data it contains except as a means for interacting with our caller.
I also think that yield_constr.one is shorter and clearer than constraint_consumer.yield_one or similar.
| } | ||
| } | ||
|
|
||
| pub trait Viewable<F> { |
There was a problem hiding this comment.
Seems semantically similar to Index; could overloading Index with the range types work?
There was a problem hiding this comment.
Yeah, this is supposed to be exactly like Index. Unfortunately, implementing Index does not work because Index::index returns a reference (index(&self, index: Idx) -> &Self::Output).
Now, in an ideal world PackedStridedView would be a reference. A slice is a reference where a pointer is augmented with extra metadata. Unfortunately, in a classic do-as-I-say-not-as-I-do Rust style, the maintainers have decided that the built-in fat pointer types are all that anyone should ever need and have not provided a way for the user to define custom ones.
So PackedStridedView is a struct and we need to return an instance of it. The instance does not exist until it is requested, so we can't return a reference as it would dangle. This precludes the use of Index and forces this ugly workaround.
There was a problem hiding this comment.
Oh I see. Please note in a brief comment. It seems like the plan is to add a IndexMove (rust-lang/rfcs/pull/2953) so there should eventually be a solution.
|
General nit: I think the |
src/util/strided_view.rs
Outdated
| // necessitates separate handling. It _could_ be done but it would also be uglier. | ||
| start_ptr: *const P::Scalar, | ||
| length: usize, | ||
| stride: usize, // This stride is in units of P::Scalar (NOT in bytes and NOT in P). |
There was a problem hiding this comment.
nit: I think it's nice to make comments like this doc comments above the field, so IDEs associate them. Also please add a similar comment for the units of length
| stride | ||
| ); | ||
|
|
||
| let start_ptr = data.as_ptr().wrapping_add(offset); |
There was a problem hiding this comment.
Could you explain the use of wrapping_ methods in this PR? Is it to skip overflow checks as a debug mode optimization?
There was a problem hiding this comment.
It's needed to avoid UB in Rust's raw pointer handling. I'll add comments!
There was a problem hiding this comment.
I've added some documentation. Let me know if it needs extra work 🙂
|
General nit: I think |
src/gates/gate.rs
Outdated
| res | ||
| } | ||
|
|
||
| /// The result is an array of length vars_batch.len() * self.num_constraints(). Constraint j for |
There was a problem hiding this comment.
nit: since these will eventually show up on docs.rs, I think it's nice to put code in backticks
There was a problem hiding this comment.
I went through the PR and inserted backticks where appropriate! There's a nonzero chance that I missed some though.
nbgl
force-pushed
the
jakub/gate-eval-mem-layout
branch
from
c0254e8
to
0e41633
Compare
nbgl
force-pushed
the
jakub/gate-eval-mem-layout
branch
from
80c9eb7
to
33a1943
Compare
This is the meat of the SIMD gate evaluation change. It changes the memory layouts associated with gate evaluation (inputs and outputs) to be matrices of field elements of shape
[[F; num_points]; some_length].It does not, by itself, enable SIMD evaluation. That PR will be easier and just a bunch of moving code around (without actually changing it much). I do not want these changes, which are more likely to be controversial, to be lost in that.