Skip to content

Commit

Permalink
feat!: Change CLI to default to new SSA code for compilation (#2032)
Browse files Browse the repository at this point in the history
feat!: Default to new SSA code
  • Loading branch information
TomAFrench authored Jul 25, 2023
1 parent 970c18e commit ce37718
Show file tree
Hide file tree
Showing 11 changed files with 34 additions and 33 deletions.
10 changes: 5 additions & 5 deletions crates/nargo_cli/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,15 @@ fn load_conf(conf_path: &Path) -> BTreeMap<String, Vec<String>> {
conf_data
}

fn generate_tests(test_file: &mut File, experimental_ssa: bool) {
fn generate_tests(test_file: &mut File, legacy_ssa: bool) {
// Try to find the directory that Cargo sets when it is running; otherwise fallback to assuming the CWD
// is the root of the repository and append the crate path
let manifest_dir = match std::env::var("CARGO_MANIFEST_DIR") {
Ok(dir) => PathBuf::from(dir),
Err(_) => std::env::current_dir().unwrap().join("crates").join("nargo_cli"),
};
// Choose the test directory depending on whether we are in the SSA refactor module or not
let test_sub_dir = if experimental_ssa { "test_data_ssa_refactor" } else { "test_data" };
// Choose the test directory depending on whether we are in the legacy SSA module or not
let test_sub_dir = if legacy_ssa { "test_data" } else { "test_data_ssa_refactor" };
let test_data_dir = manifest_dir.join("tests").join(test_sub_dir);
let config_path = test_data_dir.join("config.toml");

Expand Down Expand Up @@ -99,8 +99,8 @@ fn prove_and_verify_{test_sub_dir}_{test_name}() {{
let mut cmd = Command::cargo_bin("nargo").unwrap();
cmd.arg("--program-dir").arg(test_program_dir);
cmd.arg(if {is_workspace} {{ "test" }} else {{ "execute" }});
if {experimental_ssa} {{
cmd.arg("--experimental-ssa");
if {legacy_ssa} {{
cmd.arg("--legacy-ssa");
}};
Expand Down
8 changes: 4 additions & 4 deletions crates/nargo_cli/src/cli/check_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ fn check_from_path<B: Backend>(
&mut context,
crate_id,
compile_options.deny_warnings,
compile_options.experimental_ssa,
compile_options.legacy_ssa,
)?;

// XXX: We can have a --overwrite flag to determine if you want to overwrite the Prover/Verifier.toml files
Expand Down Expand Up @@ -220,9 +220,9 @@ pub(crate) fn check_crate_and_report_errors(
context: &mut Context,
crate_id: CrateId,
deny_warnings: bool,
experimental_ssa: bool,
legacy_ssa: bool,
) -> Result<(), ReportedErrors> {
let result = check_crate(context, crate_id, deny_warnings, experimental_ssa)
.map(|warnings| ((), warnings));
let result =
check_crate(context, crate_id, deny_warnings, legacy_ssa).map(|warnings| ((), warnings));
super::compile_cmd::report_errors(result, context, deny_warnings)
}
2 changes: 1 addition & 1 deletion crates/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ fn run_tests<B: Backend>(
&mut context,
crate_id,
compile_options.deny_warnings,
compile_options.experimental_ssa,
compile_options.legacy_ssa,
)?;

let test_functions = match context.crate_graph.crate_type(crate_id) {
Expand Down
2 changes: 1 addition & 1 deletion crates/nargo_cli/tests/test_data_ssa_refactor/rebuild.sh
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ for dir in ./*; do
dir_name=$(basename "$dir")
if [[ ! " ${exclude_fail_dirs[@]} " =~ " ${dir_name} " ]]; then
cd $dir
nargo compile --experimental-ssa main && nargo execute --experimental-ssa witness
nargo compile main && nargo execute witness
cd ..
fi
done
Expand Down
21 changes: 11 additions & 10 deletions crates/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use clap::Args;
use fm::FileId;
use noirc_abi::FunctionSignature;
use noirc_errors::{CustomDiagnostic, FileDiagnostic};
use noirc_evaluator::{create_circuit, ssa_refactor::experimental_create_circuit};
use noirc_evaluator::legacy_create_circuit;
use noirc_evaluator::ssa_refactor::create_circuit;
use noirc_frontend::graph::{CrateId, CrateName, CrateType};
use noirc_frontend::hir::def_map::{Contract, CrateDefMap};
use noirc_frontend::hir::Context;
Expand Down Expand Up @@ -39,9 +40,9 @@ pub struct CompileOptions {
#[arg(short, long)]
pub deny_warnings: bool,

/// Compile and optimize using the new experimental SSA pass
/// Compile and optimize using the old deprecated SSA pass
#[arg(long)]
pub experimental_ssa: bool,
pub legacy_ssa: bool,
}

/// Helper type used to signify where only warnings are expected in file diagnostics
Expand Down Expand Up @@ -131,7 +132,7 @@ pub fn check_crate(
context: &mut Context,
crate_id: CrateId,
deny_warnings: bool,
experimental_ssa: bool,
legacy_ssa: bool,
) -> Result<Warnings, ErrorsAndWarnings> {
// Add the stdlib before we check the crate
// TODO: This should actually be done when constructing the driver and then propagated to each dependency when added;
Expand All @@ -146,7 +147,7 @@ pub fn check_crate(
let std_crate = context.crate_graph.add_stdlib(CrateType::Library, root_file_id);
propagate_dep(context, std_crate, &CrateName::new(std_crate_name).unwrap());

context.def_interner.experimental_ssa = experimental_ssa;
context.def_interner.legacy_ssa = legacy_ssa;

let mut errors = vec![];
match context.crate_graph.crate_type(crate_id) {
Expand Down Expand Up @@ -186,7 +187,7 @@ pub fn compile_main(
crate_id: CrateId,
options: &CompileOptions,
) -> Result<(CompiledProgram, Warnings), ErrorsAndWarnings> {
let warnings = check_crate(context, crate_id, options.deny_warnings, options.experimental_ssa)?;
let warnings = check_crate(context, crate_id, options.deny_warnings, options.legacy_ssa)?;

let main = match context.get_main_function(&crate_id) {
Some(m) => m,
Expand Down Expand Up @@ -215,7 +216,7 @@ pub fn compile_contracts(
crate_id: CrateId,
options: &CompileOptions,
) -> Result<(Vec<CompiledContract>, Warnings), ErrorsAndWarnings> {
let warnings = check_crate(context, crate_id, options.deny_warnings, options.experimental_ssa)?;
let warnings = check_crate(context, crate_id, options.deny_warnings, options.legacy_ssa)?;

let contracts = context.get_all_contracts(&crate_id);
let mut compiled_contracts = vec![];
Expand Down Expand Up @@ -309,10 +310,10 @@ pub fn compile_no_check(
) -> Result<CompiledProgram, FileDiagnostic> {
let program = monomorphize(main_function, &context.def_interner);

let (circuit, debug, abi) = if options.experimental_ssa {
experimental_create_circuit(program, options.show_ssa, options.show_brillig, show_output)?
let (circuit, debug, abi) = if options.legacy_ssa {
legacy_create_circuit(program, options.show_ssa, show_output)?
} else {
create_circuit(program, options.show_ssa, show_output)?
create_circuit(program, options.show_ssa, options.show_brillig, show_output)?
};

Ok(CompiledProgram { circuit, debug, abi })
Expand Down
2 changes: 1 addition & 1 deletion crates/noirc_evaluator/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ pub struct Evaluator {
// Some of these could have been removed due to optimizations. We need this number because the
// Standard format requires the number of witnesses. The max number is also fine.
// If we had a composer object, we would not need it
pub fn create_circuit(
pub fn legacy_create_circuit(
program: Program,
enable_logging: bool,
show_output: bool,
Expand Down
2 changes: 1 addition & 1 deletion crates/noirc_evaluator/src/ssa_refactor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ pub(crate) fn optimize_into_acir(
/// This is analogous to `ssa:create_circuit` and this method is called when one wants
/// to use the new ssa module to process Noir code.
// TODO: This no longer needs to return a result, but it is kept to match the signature of `create_circuit`
pub fn experimental_create_circuit(
pub fn create_circuit(
program: Program,
enable_ssa_logging: bool,
enable_brillig_logging: bool,
Expand Down
4 changes: 2 additions & 2 deletions crates/noirc_frontend/src/hir/def_map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,10 @@ impl CrateDefMap {
// only the unconstrained one. Otherwise we want to keep only the constrained one.
ast.functions.retain(|func| {
func.def.name.0.contents.as_str() != "println"
|| func.def.is_unconstrained == context.def_interner.experimental_ssa
|| func.def.is_unconstrained != context.def_interner.legacy_ssa
});

if !context.def_interner.experimental_ssa {
if context.def_interner.legacy_ssa {
ast.module_decls.retain(|ident| {
ident.0.contents != "slice" && ident.0.contents != "collections"
});
Expand Down
2 changes: 1 addition & 1 deletion crates/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ impl<'a> Resolver<'a> {
UnresolvedType::FieldElement(comp_time) => Type::FieldElement(comp_time),
UnresolvedType::Array(size, elem) => {
let elem = Box::new(self.resolve_type_inner(*elem, new_variables));
if self.interner.experimental_ssa && size.is_none() {
if !self.interner.legacy_ssa && size.is_none() {
return Type::Slice(elem);
}
let resolved_size = self.resolve_array_size(size, new_variables);
Expand Down
10 changes: 5 additions & 5 deletions crates/noirc_frontend/src/monomorphization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ impl<'interner> Monomorphizer<'interner> {
array_contents: Vec<ast::Expression>,
element_type: ast::Type,
) -> ast::Expression {
if self.interner.experimental_ssa {
if !self.interner.legacy_ssa {
return ast::Expression::Literal(ast::Literal::Array(ast::ArrayLiteral {
contents: array_contents,
element_type,
Expand Down Expand Up @@ -438,7 +438,7 @@ impl<'interner> Monomorphizer<'interner> {
element_type: ast::Type,
location: Location,
) -> ast::Expression {
if self.interner.experimental_ssa {
if !self.interner.legacy_ssa {
return ast::Expression::Index(ast::Index {
collection,
index,
Expand Down Expand Up @@ -680,7 +680,7 @@ impl<'interner> Monomorphizer<'interner> {
HirType::Array(length, element) => {
let length = length.evaluate_to_u64().unwrap_or(0);
let element = self.convert_type(element.as_ref());
if self.interner.experimental_ssa {
if !self.interner.legacy_ssa {
return ast::Type::Array(length, Box::new(element));
}
self.aos_to_soa_type(length, element)
Expand Down Expand Up @@ -741,7 +741,7 @@ impl<'interner> Monomorphizer<'interner> {
/// Converts arrays of structs (AOS) into structs of arrays (SOA).
/// This is required since our SSA pass does not support arrays of structs.
fn aos_to_soa_type(&self, length: u64, element: ast::Type) -> ast::Type {
if self.interner.experimental_ssa {
if !self.interner.legacy_ssa {
return ast::Type::Array(length, Box::new(element));
}
match element {
Expand Down Expand Up @@ -980,7 +980,7 @@ impl<'interner> Monomorphizer<'interner> {
typ: ast::Type,
location: Location,
) -> ast::Expression {
if self.interner.experimental_ssa {
if !self.interner.legacy_ssa {
let lvalue = ast::LValue::Index { array: lvalue, index, element_type: typ, location };
return ast::Expression::Assign(ast::Assign { lvalue, expression });
}
Expand Down
4 changes: 2 additions & 2 deletions crates/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ pub struct NodeInterner {
/// TODO(#1850): This is technical debt that should be removed once we fully move over
/// to the new SSA pass which has certain frontend features enabled
/// such as slices and the removal of aos_to_soa
pub experimental_ssa: bool,
pub legacy_ssa: bool,
}

#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
Expand Down Expand Up @@ -254,7 +254,7 @@ impl Default for NodeInterner {
globals: HashMap::new(),
struct_methods: HashMap::new(),
primitive_methods: HashMap::new(),
experimental_ssa: false,
legacy_ssa: false,
};

// An empty block expression is used often, we add this into the `node` on startup
Expand Down

0 comments on commit ce37718

Please sign in to comment.