Refactor: Remove the deprecated --without-mir pipeline #424
Conversation
| @@ -83,7 +83,7 @@ impl Air for FunctionsAir { | |||
| let main_current = frame.current(); | |||
| let main_next = frame.next(); | |||
| result[0] = main_next[16] - main_current[16] * ((main_current[3] * main_current[3] * main_current[3] * main_current[3] * main_current[3] * main_current[3] * main_current[3] * main_current[1] * main_current[2] + main_current[3] * main_current[3] * (E::ONE - main_current[1]) * main_current[2] + main_current[3] * main_current[1] * (E::ONE - main_current[2]) + (E::ONE - main_current[1]) * (E::ONE - main_current[2])) * main_current[0] - main_current[0] + E::ONE); | |||
| result[1] = main_next[3] - (main_current[4] + main_current[5] + main_current[6] + main_current[7] + main_current[8] + main_current[9] + main_current[10] + main_current[11] + main_current[12] + main_current[13] + main_current[14] + main_current[15] + E::ONE) * E::from(Felt::new(2_u64)); | |||
| result[1] = main_next[3] - (E::ZERO + main_current[4] + main_current[5] + main_current[6] + main_current[7] + main_current[8] + main_current[9] + main_current[10] + main_current[11] + main_current[12] + main_current[13] + main_current[14] + main_current[15] + E::ONE) * E::from(Felt::new(2_u64)); | |||
There was a problem hiding this comment.
Question: why did this change?
There was a problem hiding this comment.
Oh, here and below the changelog does not make it clear. The two pipelines had slightly different generated graphs, so we had two different files to compare to, named functions_complex.rs and functions_complex_with_mir.rs.
I've deleted the former and renamed the later.
As to why there is a difference (e.g. for constant folding), it's because some computations are different (e.g. powers of exp operations corresponding to list comprehension indices are not yet know when doing the constant propagation. Another pass of constant prop may be needed to solve that).
| @@ -83,7 +83,7 @@ impl Air for ListComprehensionAir { | |||
| let main_current = frame.current(); | |||
| let main_next = frame.next(); | |||
| result[0] = main_current[0] - main_current[2]; | |||
| result[1] = main_current[4] - main_current[0] * E::from(Felt::new(8_u64)) * main_current[11]; | |||
| result[1] = main_current[4] - main_current[0] * E::from(Felt::new(2_u64)) * E::from(Felt::new(2_u64)) * E::from(Felt::new(2_u64)) * main_current[11]; | |||
There was a problem hiding this comment.
Similar question here: seems like maybe something is not going through constant folding?
| let pipeline = air_parser::transforms::ConstantPropagation::new(&diagnostics) | ||
| .chain(mir::passes::AstToMir::new(&diagnostics)) | ||
| .chain(mir::passes::Inlining::new(&diagnostics)) | ||
| .chain(mir::passes::Unrolling::new(&diagnostics)) | ||
| .chain(air_ir::passes::MirToAir::new(&diagnostics)) | ||
| .chain(air_ir::passes::BusOpExpand::new(&diagnostics)); |
There was a problem hiding this comment.
Similar comment as above: it is great that we show the flexibility here, but would also be good to show how this can be done in a way that doesn't require the user to know about all the different pipelines and their required order.
air-script/README.md
Outdated
| @@ -9,14 +9,15 @@ An in-depth description of AirScript is available in the full AirScript [documen | |||
| The compiler has three stages, which can be imported and used independently or together. | |||
There was a problem hiding this comment.
nit: this should probably say "four stages" now?
Indeed this as been raised by @bobbinth as well, there is a regression in MIR as, for instance, powers of exp operations corresponding to list comprehension indices are not yet know when doing the AST constant propagation. Another pass of constant propagation after MIR would solve that, here is the corresponding issue: #427 |
This PR closes #405.