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

Prepare to call jit from block machine. #2098

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Conversation

chriseth
Copy link
Member

@chriseth chriseth commented Nov 15, 2024

This PR performs preliminary preparations in the block machine so that it will be able to JIT-compile and evaluate lookups into this machine given a certain combination of "known inputs".

@chriseth chriseth changed the title Call jit from block Prepare to call jit from block machine. Nov 15, 2024
@chriseth chriseth marked this pull request as ready for review November 15, 2024 17:17
@chriseth chriseth changed the base branch from main to improve_finalizable November 15, 2024 17:18
Base automatically changed from improve_finalizable to main November 15, 2024 17:49
Self { data, row_offset }
}

pub fn get(&self, row: i32, col: u32) -> T {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the 32 bit values here provide a performance advantage. Any opinions?

Copy link
Collaborator

@Schaeff Schaeff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of this is a bit too deep for me to judge, so just a few comments.

@@ -13,7 +13,7 @@ use crate::witgen::rows::Row;
/// Sequence of rows of field elements, stored in a compact form.
/// Optimized for contiguous column IDs, but works with any combination.
#[derive(Clone)]
struct CompactData<T: FieldElement> {
pub struct CompactData<T: FieldElement> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub struct CompactData<T: FieldElement> {
pub struct CompactData<T> {


pub fn set(&mut self, row: usize, col: u64, value: T) {
let idx = self.index(row, col);
assert!(!self.known_cells[idx] || self.data[idx] == value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that we sometimes set the same value twice?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or that there's a default value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK to set the value twice if it's the same value. We can maybe make this more strict later, though.

The "default" value is "not known", internally it will be represented by zero.

/// A mutable reference into CompactData that is meant to be used
/// only for a certain block of rows, starting from row index zero.
/// It allows negative row indices as well.
pub struct CompactDataRef<'a, T: FieldElement> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub struct CompactDataRef<'a, T: FieldElement> {
pub struct CompactDataRef<'a, T> {

?

@@ -70,6 +71,9 @@ pub struct BlockMachine<'a, T: FieldElement> {
/// Cache that states the order in which to evaluate identities
/// to make progress most quickly.
processing_sequence_cache: ProcessingSequenceCache,
/// The JIT processor for this machine, i.e. the component that tries to generate
/// witgen code based on which elements of the connection are known.
jit_processer: JitProcessor<'a, T>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
jit_processer: JitProcessor<'a, T>,
jit_processor: JitProcessor<'a, T>,

?

) -> Vec<LookupCell<'c, T>> {
self.left
.iter()
.zip(input_output_data.iter_mut())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would zip_eq hurt performance?

@@ -215,6 +273,41 @@ impl<T: FieldElement> FinalizableData<T> {
}
}

/// Returns an iterator over the values known in that row together with the PolyIDs.
pub fn known_values_in_row(&self, row: usize) -> Box<dyn Iterator<Item = (PolyID, T)> + '_> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just wondering if we need the dynamic output, can self.location_of_row(row) be anything in practice?

@leonardoalt
Copy link
Member

has conflicts

@leonardoalt
Copy link
Member

ready for review again?

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

Successfully merging this pull request may close these issues.

3 participants