Codegen option for generic constraint evaluation format #104

Fumuran · 2022-12-15T18:55:13Z

Related issue: #56

Things left to implement:

boundary constraints (for expressions and outputs arrays)
EXP operation emulation
Random Values support

Things to implement in later version:

Periodic Columns support
Constraint merging logic

bobbinth · 2022-12-16T21:13:25Z

One quick comment: this should go into its own crate (not winterfell codegen). So, instead of being under codegen/winterfell it should be under codegen/gce (for "generic constraint evaluator"). But I'm also open to other suggestions. @grjte - how do you think we should name it?

grjte · 2022-12-19T16:20:20Z

So, instead of being under codegen/winterfell it should be under codegen/gce (for "generic constraint evaluator"). But I'm also open to other suggestions. @grjte - how do you think we should name it?

I think gce works - I've thought a bit, but there's nothing else I prefer.

grjte

This is a partial review. I left several preliminary comments inline related to refactoring. After that, it should be easier to assess some of the other design decisions here.

grjte · 2022-12-19T16:24:25Z

air-script/src/cli/transpile.rs

+        generate_constraint_evaluation(&ir, 2).unwrap();
+


A couple things here -

In general, it would be better to use expect than unwrap in production code.

Now that we have multiple codegen options, we should update the CLI to accept another input that specifies the target output type. This should be done in a separate PR, though. Can you create an issue for this and add it to the "nice to have" list for Tracking issue: core functionality for Miden VM constraints #75?

Based on those 2 points of feedback, I think we should remove this from here.

grjte · 2022-12-19T16:25:04Z

codegen/gce/Cargo.toml

+[package]
+name = "air-codegen-gce"
+version = "0.1.0"
+description="Codegen for generic constraint evaluation"


nit: I would adjust this to "Code generation for the generic constraint evaluation format."

grjte · 2022-12-19T16:30:42Z

codegen/gce/src/generic_constraint_evaluation.rs

+/// Holds data for JSON generation
+#[derive(Default, Debug)]
+pub struct Evaluation {
+    num_polys: u8,


I think we should probably use u16 here. Although u8 is right for Miden, other VMs may need more (e.g. zk EVMs).

grjte · 2022-12-19T16:33:31Z

codegen/gce/src/generic_constraint_evaluation.rs

+        // constants
+        //----------


nit: let's use the same format for separators that we follow elsewhere in the codebase and in the VM.

grjte · 2022-12-19T16:36:08Z

codegen/gce/src/generic_constraint_evaluation.rs

+        // expressions (boundary constraints)
+        //------------
+        for main_constraints in &boundary_constraints_vec[..2] {
+            for main_constraint in *main_constraints {
+                handle_boundary_operation(
+                    ir,
+                    main_constraint,
+                    &mut expressions,
+                    &mut output,
+                    &constants,
+                    &const_public_type_map,
+                )?;
+            }
+        }
+        for aux_constraints in &boundary_constraints_vec[2..] {
+            for aux_constraint in *aux_constraints {
+                handle_boundary_operation(
+                    ir,
+                    &(
+                        aux_constraint.0 + ir.num_polys().0 as usize,
+                        aux_constraint.1,
+                    ),
+                    &mut expressions,
+                    &mut output,
+                    &constants,
+                    &const_public_type_map,
+                )?;
+            }
+        }


These loops are almost exact duplicates. Can you pull this out into a helper function?

grjte · 2022-12-19T16:57:42Z

codegen/gce/src/tests/mod.rs

+#[test]
+fn constraint_evaluation() {
+    let generated_air = get_ir("src/tests/constraint_evaluation.air".to_string()).unwrap();
+    let evaluation = Evaluation::new(&generated_air, 2).expect("Evaluation creation failed");
+    evaluation
+        .generate_evaluation_file()
+        .expect("File generation failed");
+}


Thanks for adding a test! Since this is a test of the full transpilation from parser to IR to codegen, I think it doesn't belong in this crate. This crate should contain tests that are only for gce codegen.

Instead, it would be great to update the existing transpilation tests in the air-script crate so that they test for both Winterfell and gce. I would do this as follows:

1. Update the Test struct to something like the following in air-script/tests/helpers.rs`

pub struct Test { source: String, parsed: Source, ir: AirIR } impl Test { pub fn new(input_path: String) -> Self { // load source input from file let source = fs::read_to_string(&self.input_path).map_err(|err| { TestError::IO(format!( "Failed to open input file `{:?}` - {}", input_path, err )) })?; // parse the input file to the internal representation let parsed = parse(source.as_str()).map_err(|_| { TestError::Parse(format!( "Failed to parse the input air file at {}", input_path )) })?; let ir = AirIR::from_source(&parsed).map_err(|_| { TestError::IR(format!( "Failed to convert the input air file at {} to IR representation", input_path )) })?; Test { source, parsed, ir } } pub fn generate_winterfell(&self) -> Result<String, TestError> { // generate Rust code targeting Winterfell let codegen = WinterfellCodeGenerator::new(&self.ir); Ok(codegen.generate()) } pub fn generate_gce(&self) -> Result<String, TestError> { // generate Rust code targeting Winterfell let codegen = GCECodeGenerator::new(&self.ir); Ok(codegen.generate()) } }

2. Update the tests to do code generation for both types in air-script/tests/main.rs, for example:

#[test] fn winterfell_aux_trace() { // Winterfell target let generated_air = Test::new("tests/aux_trace/aux_trace.air".to_string()) .generate_winterfell() .unwrap(); let expected = expect_file!["aux_trace/aux_trace.rs"]; expected.assert_eq(&generated_air); } #[test] fn gce_aux_trace() { // GCE target let generated_air = Test::new("tests/aux_trace/aux_trace.air".to_string()) .generate_gce() .unwrap(); let expected = expect_file!["aux_trace/aux_trace.json"]; expected.assert_eq(&generated_air); }

grjte · 2022-12-19T16:58:29Z

codegen/winterfell/Cargo.toml

+
+[dev-dependencies]
+parser = { package = "air-parser", path = "../../parser", version = "0.1.0" }


Why is this necessary?

codegen/gce/src/lib.rs

grjte · 2022-12-19T17:29:26Z

ir/src/lib.rs

+                    // set the overall trace columns count
+                    num_polys = (columns.main_cols.len() as u8, columns.aux_cols.len() as u8);


This won't quite work, because we could have more than one TraceCols source section (e.g. once we support modules).

Instead of doing it here and initializing above, maybe we should collect this information inside the symbol table, the way that we collect the num_trace_segments information. Then you could update the into_declarations method to pass the information you need to initialize num_polys. I think this will encapsulate things a bit more cleanly.

grjte · 2022-12-19T17:32:42Z

ir/src/lib.rs

+    // I don't think that adding a new field just for generic constraint evaluation worth it.
+    // TODO: get rid of this field
+    // amount of named columns: (main, aux)
+    num_polys: (u8, u8),


I like that you're considering whether or not we want this, but I actually do think we need to add something here, because I don't think there's any other way to get this information.

One note though - currently we only have 2 trace segments, but we've actually been making the IR more generic to support more than 2 segments, so this should probably be a vector to be consistent with those changes.

grjte

Thanks @Overcastan! This refactor looks a lot cleaner. I've done a partial review. Can you please address these comments and add doc comments to functions where they're missing? It'll make it easier to review the rest of this.

grjte · 2023-01-09T08:58:54Z

air-script/tests/helpers.rs

+    }
+
+    pub fn generate_gce(&self, extension_degree: u8, path: &str) -> Result<(), TestError> {
+        // generate Rust code targeting Winterfell


nit: this comment is incorrect.

grjte · 2023-01-09T08:59:20Z

air-script/tests/helpers.rs

+        codegen.generate()
+    }
+
+    pub fn generate_gce(&self, extension_degree: u8, path: &str) -> Result<(), TestError> {


Can you add a doc comment to this function and the generate_winterfell function?

grjte · 2023-01-09T09:01:24Z

air-script/tests/helpers.rs

-    pub fn new(input_path: String) -> Self {
-        Test { input_path }
-    }
-
-    pub fn transpile(&self) -> Result<String, TestError> {
+    pub fn new(input_path: String) -> Result<Self, TestError> {


I'd prefer to keep these separate. The idea is that if they're separate then in the future we could offer different options - full transpilation via transpile, or only testing the parsing or ir generation steps

grjte · 2023-01-09T09:03:29Z

air-script/tests/helpers.rs

-use codegen_winter::CodeGenerator;
+use codegen_gce::GCECodeGenerator;
+use codegen_winter::WinterfellCodeGenerator;


Once we have both of these in the air_script library, you can import as:

use air_script::{GceCodeGenerator, WinterfellCodeGenerator};

I think we should actually do the same with the IR and parser inputs, so we would have:

use air_script::{parse, AirIR, GceCodeGenerator, WinterfellCodeGenerator};

grjte · 2023-01-09T09:18:44Z

air-script/src/lib.rs

-pub use codegen_winter::CodeGenerator;
+pub use codegen_winter::WinterfellCodeGenerator;


We should also export the GceCodeGenerator.

Also, I'd actually prefer to name these as CodeGenerator in their respective crates and then alias the imports, i.e.:

pub use codegen_gce::CodeGenerator as GceCodeGenerator; pub use codegen_winter::CodeGenerator as WinterfellCodeGenerator;

grjte · 2023-01-09T09:25:50Z

air-script/tests/main.rs

-use expect_test::expect_file;
+use std::fs::File;
+use std::io::prelude::*;


I don't think we need to change the ordering and spacing here. These three imports are all external imports. What was your reasoning for this?

Fixed. There was no reason, it looks like the auto import just arranged them like this, and I overlooked

grjte · 2023-01-09T09:31:50Z

air-script/tests/main.rs

+fn gce_aux_trace() {
+    Test::new("tests/aux_trace/aux_trace.air".to_string())
+        .unwrap()
+        .generate_gce(2, "tests/aux_trace/generated_aux_trace.json")


Let's not unwrap twice in the same chain here. Instead, we can do something like this.

let test = Test::new("tests/aux_trace/aux_trace.air".to_string()).unwrap(); assert!(test .generate_gce(2, "tests/aux_trace/generated_aux_trace.json") .is_ok());

grjte · 2023-01-09T09:38:15Z

air-script/tests/main.rs

+    let mut file = File::open("tests/aux_trace/generated_aux_trace.json").unwrap();
+    let mut contents = String::new();
+    file.read_to_string(&mut contents).unwrap();
+
+    expected.assert_eq(&contents);
+}


This generated file should be deleted at the end of the test, not checked into the repo. In general, think of tests as having 3 stages: "setup", "execute", "tear down", where the tear down step leaves the system as you found it.

Use a variable for "tests/aux_trace/generated_aux_trace.json" instead of typing it twice

In general, I would probably tidy up the handling of the paths a bit too, so I would combine all of these suggestions into something like:

#[test] fn gce_aux_trace() { let test_path = "tests/aux_trace/aux_trace"; let result_file = "tests/aux_trace/generated_aux_trace.json"; let expected = expect_file![[test_path, "json"].join(".")]; let test = Test::new([test_path, "air"].join(".")).unwrap(); assert!(test.generate_gce(2, result_file).is_ok()); let mut file = File::open(result_file).unwrap(); let mut result = String::new(); file.read_to_string(&mut result).unwrap(); expected.assert_eq(&result); // delete the generated result file remove_file(result_file).unwrap(); }

grjte · 2023-01-09T09:54:38Z

codegen/gce/src/lib.rs

+use error::ConstraintEvaluationError;
+
+mod helpers;
+use helpers::{acumulate_constants, count_boundary_rand_values, Expression, NodeType};


nit: there's a typo here - this should be accumulate_constants

codegen/gce/src/lib.rs

grjte

Another partial review :) The testing structure looks good now except for one nit inline. The test reference code is incorrect, though. I would start by thinking through what the reference json should be to make sure it's clear. I left a few examples inline, but please let me know if you have questions

grjte · 2023-01-11T10:50:19Z

air-script/tests/main.rs

+    let mut contents = String::new();
+    file.read_to_string(&mut contents)
+        .expect("Failed to read form file");
+    fs::remove_file(result_file).expect("Failed to remove file");


nit: since this file removal is a "tear down" step, it should be at the end of the test, after the assertion that constitutes the actual test. That will be true for all of the tests in this file.

grjte · 2023-01-11T10:52:46Z

air-script/tests/main.rs

+#[test]
+#[ignore] // exponentiation for boundary constraints is not ready
+fn gce_exponentiation() {
+    let test_path = "tests/exponentiation/exponentiation";
+    let result_file = "tests/exponentiation/generated_exponentiation.json";
+
+    let test = Test::new([test_path, "air"].join("."));
+    test.generate_gce(2, result_file)
+        .expect("GCE generation failed");
+
+    let expected = expect_file![[test_path, "json"].join(".").trim_start_matches("tests/")];
+
+    let mut file = File::open(result_file).expect("Failed to open file");
+    let mut contents = String::new();
+    file.read_to_string(&mut contents)
+        .expect("Failed to read form file");
+    fs::remove_file(result_file).expect("Failed to remove file");
+
+    expected.assert_eq(&contents);
+}
+


Since this isn't done, can we move this test (and the related files) to a separate branch? That way they can be handled in a separate PR in the future and we can keep this review focused on things that are ready

grjte · 2023-01-11T11:06:47Z

codegen/gce/src/helpers.rs

+// I think we can allow non camel case type since we translate it directly to string in node
+// reference type, where we don't use camel case
+/// Stroes node type required in [NodeReference] struct
+#[allow(non_camel_case_types, clippy::upper_case_acronyms)]
+#[derive(Debug, Clone)]
+pub enum NodeType {
+    POL,
+    POL_NEXT,
+    VAR,
+    CONST,
+    EXPR,
+}


Your argument is reasonable, but I would probably stick with Rust conventions and then just implement Display for NodeType as well. In general, I prefer not to make exceptions to conventions unless there's a really good reason for it. In this case, I think it's pretty much neutral, so I'd lean in favor of the convention

grjte · 2023-01-11T11:08:03Z

codegen/gce/src/helpers.rs

+// TODO: change String to &str (Or should I create another enum?)
+/// Stores data used in JSON generation
+#[derive(Debug)]
+pub struct Expression {
+    pub op: String,
+    pub lhs: NodeReference,
+    pub rhs: NodeReference,
+}


Yep, I would create another enum

grjte · 2023-01-11T11:09:27Z

codegen/gce/src/helpers.rs

+/// Counts overall number of random values in boundary constraints
+pub fn count_boundary_rand_values<'a>(
+    expr: &'a BoundaryExpr,
+    max_random_values_index: &'a mut usize,
+) {
+    use BoundaryExpr::*;
+    match expr {
+        Rand(i) => *max_random_values_index = *max_random_values_index.max(&mut (*i + 1)),
+        Add(l, r) | Sub(l, r) | Mul(l, r) => {
+            count_boundary_rand_values(l, max_random_values_index);
+            count_boundary_rand_values(r, max_random_values_index);
+        }
+        _ => {}
+    }
+}
+


Instead of having this "helpers" file, I would put the structs (and related implementations) into the lib file and move these helper functions to a utils.rs file

grjte · 2023-01-11T12:07:30Z

air-script/tests/aux_trace/aux_trace.json

+	"expressions": [
+		{"op": "ADD", "lhs": {"type": "POL", "index": 1}, "rhs": {"type": "POL", "index": 2}},
+		{"op": "SUB", "lhs": {"type": "POL_NEXT", "index": 0}, "rhs": {"type": "EXPR", "index": 0}},
+		{"op": "ADD", "lhs": {"type": "POL", "index": 2}, "rhs": {"type": "POL_NEXT", "index": 0}},
+		{"op": "SUB", "lhs": {"type": "POL_NEXT", "index": 1}, "rhs": {"type": "EXPR", "index": 2}},
+		{"op": "ADD", "lhs": {"type": "POL", "index": 0}, "rhs": {"type": "POL", "index": 1}},
+		{"op": "SUB", "lhs": {"type": "POL", "index": 2}, "rhs": {"type": "EXPR", "index": 4}},
+		{"op": "ADD", "lhs": {"type": "POL", "index": 0}, "rhs": {"type": "VAR", "index": 16}},
+		{"op": "ADD", "lhs": {"type": "EXPR", "index": 6}, "rhs": {"type": "POL", "index": 1}},
+		{"op": "ADD", "lhs": {"type": "EXPR", "index": 7}, "rhs": {"type": "VAR", "index": 17}},
+		{"op": "MUL", "lhs": {"type": "POL", "index": 3}, "rhs": {"type": "EXPR", "index": 8}},
+		{"op": "SUB", "lhs": {"type": "POL_NEXT", "index": 3}, "rhs": {"type": "EXPR", "index": 9}},
+		{"op": "ADD", "lhs": {"type": "POL", "index": 2}, "rhs": {"type": "VAR", "index": 16}},
+		{"op": "MUL", "lhs": {"type": "POL_NEXT", "index": 4}, "rhs": {"type": "EXPR", "index": 11}},
+		{"op": "SUB", "lhs": {"type": "POL", "index": 4}, "rhs": {"type": "EXPR", "index": 12}},
+		{"op": "SUB", "lhs": {"type": "POL", "index": 0}, "rhs": {"type": "CONST", "index": 0}},
+		{"op": "SUB", "lhs": {"type": "POL", "index": 1}, "rhs": {"type": "CONST", "index": 0}},
+		{"op": "SUB", "lhs": {"type": "POL", "index": 3}, "rhs": {"type": "CONST", "index": 0}},
+		{"op": "SUB", "lhs": {"type": "POL", "index": 4}, "rhs": {"type": "VAR", "index": 16}},
+		{"op": "SUB", "lhs": {"type": "POL", "index": 3}, "rhs": {"type": "CONST", "index": 0}},
+		{"op": "SUB", "lhs": {"type": "POL", "index": 4}, "rhs": {"type": "CONST", "index": 0}}
+	],
+	"outputs": [1, 3, 5, 10, 13, 14, 15, 16, 17, 18, 19]


This output isn't correct, since the "outputs" should be an array of node references.

So for example, the first transition constraint enf a' = b + c would be expressed by the following:

"expressions": [ {"op": "ADD", "lhs": {"type": "POL", "index": 1}, "rhs": {"type": "POL", "index": 2}}, {"op": "SUB", "lhs": {"type": "POL_NEXT", "index": 0}, "rhs": {"type": "REF", "index": 0}} ], "outputs": [ {"type": "POL_NEXT", "index": 0}, {"type": "POL", "index": 1}, {"type": "POL", "index": 2}, {"type": "CONST", "index": 0}, {"type": "REF", "index": 0} ]

In this case, the first item in the expressions array represents b+c, which is then added to outputs as a REF. The second item in the expressions array represents a' - (b+c)

grjte · 2023-01-11T12:17:52Z

codegen/gce/src/lib.rs

+        // TODO #2: currently I add all found constants in the vector. Should I add only unique ones,
+        // since I'll get constants by their value, not index?
+        let constants = set_constants(ir, &mut const_public_type_map, boundary_constraints_vec);


I'm not sure if I understood this question correctly, but we should be referencing constants by index, so yes, it would be best to only add unique ones.

In other words, if we had a'= a + 1, we would have something like the following:

... "constants": [1] "expressions": [ {"op":"ADD", "lhs": {"type": "POL", "index": 0}, "rhs": {"type": "CONST", "index": 0}}, {"op": "SUB", "lhs": {"type": "POL_NEXT", "index": 0}, "rhs": {"type": "REF", "index": 0}} ], "outputs": [ {"type": "POL_NEXT", "index": 0}, {"type": "POL", "index": 0}, {"type": "CONST", "index": 0}, {"type": "REF", "index": 0} ]

grjte

@Overcastan thank you! The refactor you've done looks good. It's much easier to follow. This is a partial review with a few nits and a small suggested adjustment to the pattern you're using to build the expressions & outputs

grjte · 2023-02-24T16:35:41Z

air-script/tests/main.rs

 mod helpers;
 use helpers::Test;


nit: let's put a blank line between the module declaration and the imports

grjte · 2023-02-24T16:37:38Z

codegen/gce/Cargo.toml

@@ -0,0 +1,17 @@
+[package]
+name = "air-codegen-gce"
+version = "0.1.0"


Let's set this as 0.2.0 so all crates have the same version

grjte · 2023-02-24T16:40:18Z

codegen/gce/src/error.rs

+
+    pub fn invalid_constant_type(name: &str, constant_type: &str) -> Self {
+        ConstraintEvaluationError::InvalidConstantType(format!(
+            "Invalid type of constant \"{name}\". {constant_type} exprected."


nit: typo in "expected". Let's also reorder to the following: "Invalid type of constant "{name}". Expected "{constant_type}"."

grjte · 2023-02-24T16:41:17Z

codegen/gce/src/error.rs

+
+    pub fn operation_not_found(index: usize) -> Self {
+        ConstraintEvaluationError::OperationNotFound(format!(
+            "Operation with index {index} does not match the expression in the expressions JSON array"


nit: let's adjust this to: "Operation with index {index} does not match the expression in the JSON expressions array."

grjte · 2023-02-24T16:41:53Z

codegen/gce/src/error.rs

+impl ConstraintEvaluationError {
+    pub fn invalid_trace_segment(segment: u8) -> Self {
+        ConstraintEvaluationError::InvalidTraceSegment(format!(
+            "Trace segment {segment} is invalid"
+        ))
+    }
+
+    pub fn constant_not_found(name: &str) -> Self {
+        ConstraintEvaluationError::ConstantNotFound(format!("Constant \"{name}\" not found"))
+    }


nit: for consistency, let's add "." at the end of the error message sentences

grjte · 2023-02-24T17:10:57Z

codegen/gce/src/lib.rs

+        // maps indexes in Node vector in AlgebraicGraph and in `expressions` JSON array
+        let mut expressions_map = BTreeMap::new();


This isn't used at this level, so it can be removed here and created directly when the ExpressionsHandler is instantiated

grjte · 2023-02-24T17:13:08Z

codegen/gce/src/lib.rs

+        let mut expressions_handler = ExpressionsHandler::new(ir, &constants, &mut expressions_map);
+
+        let mut expressions = expressions_handler.get_expressions()?;
+        // vector of `expressions` indexes
+        let outputs = expressions_handler.get_outputs(&mut expressions)?;


Instead of this pattern where you get the expressions and then pass them back in, I would keep them in the ExpressionsHandler. I suggest adjusting to a builder pattern. We could rename ExpressionsHandler to GceBuilder, add a method which handles building the expressions and outputs, and then add an into method that consumes the builder and yields the expressions and outputs. (This would also reflect the pattern that we'll be moving the IR to in #150)

Something like:

pub fn new(ir: &AirIR, extension_degree: u8) -> Result<Self, ConstraintEvaluationError> { let num_polys = set_num_polys(ir, extension_degree); let num_variables = set_num_variables(ir); let constants = set_constants(ir); let gce_builder = GceBuilder::new(); gce_builder.build(ir, &constants)?; let (expressions, outputs) = gce_builder.into_gce(); Ok(CodeGenerator { num_polys, num_variables, constants, expressions, outputs, }) }

grjte · 2023-02-24T17:19:34Z

codegen/gce/src/expressions.rs

+pub struct ExpressionsHandler<'a> {
+    ir: &'a AirIR,
+    constants: &'a [u64],
+    // maps indexes in Node vector in AlgebraicGraph and in `expressions` JSON array
+    expressions_map: &'a mut BTreeMap<usize, usize>,
+}


As noted, the expressions_map can be instantiated in the new method, so it no longer needs to be marked as a mut borrow with a lifetime.

I think we should also add the following. As mentioned, we can declare them in new and then populate them in a build method.

expressions: Vec<ExpressionJson>, outputs: Vec<usize>,

I would also probably remove the AirIR and constants borrows here and just pass them to the build method.

For the build method, we could do something like the following:

pub fn build( &mut self, ir: &AirIR, constants: &[u64], ) -> Result<(), ConstraintEvaluationError> { self.build_expressions(ir, constants)?; self.build_outputs(ir)?; Ok(()) }

grjte · 2023-02-24T17:20:20Z

codegen/gce/src/expressions.rs

+    /// Parses expressions in transition graph's Node vector, creates [Expression] instances and pushes
+    /// them to the `expressions` vector.
+    pub fn get_expressions(&mut self) -> Result<Vec<ExpressionJson>, ConstraintEvaluationError> {


We can change the signature to the following to use the builder pattern I suggested:

fn build_expressions( &mut self, ir: &AirIR, constants: &[u64], ) -> Result<(), ConstraintEvaluationError> {

grjte · 2023-02-24T17:20:54Z

codegen/gce/src/expressions.rs

+    /// Fills the `outputs` vector with indexes from `expressions` vector according to the `expressions_map`.
+    pub fn get_outputs(
+        &self,
+        expressions: &mut Vec<ExpressionJson>,
+    ) -> Result<Vec<usize>, ConstraintEvaluationError> {


We can change the signature to the following to implement the builder pattern I suggested:

fn build_outputs(&self, ir: &AirIR) -> Result<Vec<usize>, ConstraintEvaluationError> {

Fumuran linked an issue Dec 15, 2022 that may be closed by this pull request

Implement codegen option for generic constraint evaluation format #56

Open

tohrnii mentioned this pull request Dec 15, 2022

Tracking PR for v0.2 release #79

Merged

14 tasks

Fumuran removed a link to an issue Dec 18, 2022

Implement codegen option for generic constraint evaluation format #56

Open

Fumuran requested review from grjte, bobbinth and tohrnii and removed request for grjte and bobbinth December 18, 2022 13:01

Fumuran marked this pull request as ready for review December 18, 2022 13:15

grjte requested review from Al-Kindi-0 and removed request for bobbinth December 19, 2022 13:37

grjte suggested changes Dec 19, 2022

View reviewed changes

Fumuran marked this pull request as draft December 20, 2022 14:47

Fumuran removed request for tohrnii and Al-Kindi-0 December 20, 2022 14:47

Fumuran mentioned this pull request Dec 22, 2022

Update the CLI to specify the target output type #107

Closed

Fumuran force-pushed the andrew-generic-constraint-evaluation-codegen branch 4 times, most recently from 2d30954 to 0a66ed6 Compare January 5, 2023 12:25

Fumuran marked this pull request as ready for review January 5, 2023 12:28

Fumuran requested review from grjte, tohrnii and Al-Kindi-0 January 5, 2023 12:28

Fumuran force-pushed the andrew-generic-constraint-evaluation-codegen branch from 0a66ed6 to 02dba70 Compare January 6, 2023 14:22

grjte suggested changes Jan 9, 2023

View reviewed changes

Fumuran requested a review from grjte January 9, 2023 14:43

grjte suggested changes Jan 11, 2023

View reviewed changes

Fumuran force-pushed the andrew-generic-constraint-evaluation-codegen branch from f43e251 to bae4fcc Compare January 11, 2023 14:32

Fumuran marked this pull request as draft January 11, 2023 17:23

Fumuran force-pushed the andrew-generic-constraint-evaluation-codegen branch 3 times, most recently from 2a55bc6 to 4ee1a76 Compare February 8, 2023 21:42

Fumuran marked this pull request as ready for review February 9, 2023 11:09

Fumuran requested a review from grjte February 9, 2023 11:16

Fumuran force-pushed the andrew-generic-constraint-evaluation-codegen branch from 4ee1a76 to 7a55422 Compare February 10, 2023 12:52

Fumuran marked this pull request as draft February 23, 2023 15:14

Fumuran removed request for tohrnii, Al-Kindi-0 and grjte February 23, 2023 15:15

Fumuran force-pushed the andrew-generic-constraint-evaluation-codegen branch from 7a55422 to fa144de Compare February 23, 2023 15:58

Fumuran marked this pull request as ready for review February 23, 2023 16:58

Fumuran requested review from grjte, tohrnii and Al-Kindi-0 February 23, 2023 16:58

Fumuran force-pushed the andrew-generic-constraint-evaluation-codegen branch from 1b1afaf to 644b6b0 Compare February 24, 2023 16:03

grjte suggested changes Feb 24, 2023

View reviewed changes

Fumuran requested a review from grjte February 25, 2023 16:24

feat: add GCE codegen

402fa0b

Fumuran force-pushed the andrew-generic-constraint-evaluation-codegen branch from f72569a to bda7dc5 Compare February 27, 2023 12:29

Fumuran added 3 commits February 28, 2023 15:03

test: add GCE testing

2422925

chore: remove unused errors, update error fromat

6bc5038

refactor: update ExpressionHandler to match builder pattern

e839cfa

Fumuran force-pushed the andrew-generic-constraint-evaluation-codegen branch from bda7dc5 to e839cfa Compare February 28, 2023 12:09

grjte removed their request for review June 12, 2023 13:56


		[dev-dependencies]
		parser = { package = "air-parser", path = "../../parser", version = "0.1.0" }

		// set the overall trace columns count
		num_polys = (columns.main_cols.len() as u8, columns.aux_cols.len() as u8);

		pub use codegen_winter::CodeGenerator;
		pub use codegen_winter::WinterfellCodeGenerator;

		// maps indexes in Node vector in AlgebraicGraph and in `expressions` JSON array
		let mut expressions_map = BTreeMap::new();

Codegen option for generic constraint evaluation format #104

Are you sure you want to change the base?

Codegen option for generic constraint evaluation format #104

Conversation

Fumuran commented Dec 15, 2022 • edited Loading

bobbinth commented Dec 16, 2022

grjte commented Dec 19, 2022

grjte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1. Update the Test struct to something like the following in air-script/tests/helpers.rs`

2. Update the tests to do code generation for both types in air-script/tests/main.rs, for example:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grjte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grjte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grjte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grjte Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fumuran commented Dec 15, 2022 •

edited

Loading

1. Update the `Test` struct to something like the following in air-script/tests/helpers.rs`

2. Update the tests to do code generation for both types in `air-script/tests/main.rs`, for example:

grjte Feb 24, 2023 •

edited

Loading