Skip to content

Commit

Permalink
chore: Use --show-output flag on execution rather than compilation (#…
Browse files Browse the repository at this point in the history
…2116)

* move show-output to occur on execute rather than compilation

* remove assert(false) from test

* fix compile err

* report compile errors in tests

* aupdate failing constraint test

* change comment and link issue
  • Loading branch information
vezenovm authored Aug 2, 2023
1 parent 1c21d0c commit 27ab78f
Show file tree
Hide file tree
Showing 10 changed files with 46 additions and 61 deletions.
3 changes: 2 additions & 1 deletion crates/nargo/src/ops/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub fn execute_circuit<B: BlackBoxFunctionSolver + Default>(
_backend: &B,
circuit: Circuit,
initial_witness: WitnessMap,
show_output: bool,
) -> Result<WitnessMap, NargoError> {
let mut acvm = ACVM::new(B::default(), circuit.opcodes, initial_witness);

Expand All @@ -23,7 +24,7 @@ pub fn execute_circuit<B: BlackBoxFunctionSolver + Default>(
}
ACVMStatus::Failure(error) => return Err(error.into()),
ACVMStatus::RequiresForeignCall(foreign_call) => {
let foreign_call_result = ForeignCall::execute(&foreign_call)?;
let foreign_call_result = ForeignCall::execute(&foreign_call, show_output)?;
acvm.resolve_pending_foreign_call(foreign_call_result);
}
}
Expand Down
5 changes: 4 additions & 1 deletion crates/nargo/src/ops/foreign_calls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,14 @@ impl ForeignCall {

pub(crate) fn execute(
foreign_call: &ForeignCallWaitInfo,
show_output: bool,
) -> Result<ForeignCallResult, ForeignCallError> {
let foreign_call_name = foreign_call.function.as_str();
match Self::lookup(foreign_call_name) {
Some(ForeignCall::Println) => {
Self::execute_println(&foreign_call.inputs)?;
if show_output {
Self::execute_println(&foreign_call.inputs)?;
}
Ok(ForeignCallResult { values: vec![] })
}
Some(ForeignCall::Sequence) => {
Expand Down
2 changes: 1 addition & 1 deletion crates/nargo_cli/src/cli/execute_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ pub(crate) fn execute_program<B: Backend>(
debug_data: Option<(DebugInfo, Context)>,
) -> Result<WitnessMap, CliError<B>> {
let initial_witness = abi.encode(inputs_map, None)?;
let solved_witness_err = nargo::ops::execute_circuit(backend, circuit, initial_witness);
let solved_witness_err = nargo::ops::execute_circuit(backend, circuit, initial_witness, true);
match solved_witness_err {
Ok(solved_witness) => Ok(solved_witness),
Err(err) => {
Expand Down
9 changes: 6 additions & 3 deletions crates/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,17 @@ fn run_test<B: Backend>(
show_output: bool,
config: &CompileOptions,
) -> Result<(), CliError<B>> {
let mut program = compile_no_check(context, show_output, config, main)
.map_err(|_| CliError::Generic(format!("Test '{test_name}' failed to compile")))?;
let mut program = compile_no_check(context, config, main).map_err(|err| {
noirc_errors::reporter::report_all(&context.file_manager, &[err], config.deny_warnings);
CliError::Generic(format!("Test '{test_name}' failed to compile"))
})?;

// Note: We could perform this test using the unoptimized ACIR as generated by `compile_no_check`.
program.circuit = optimize_circuit(backend, program.circuit).unwrap().0;

// Run the backend to ensure the PWG evaluates functions like std::hash::pedersen,
// otherwise constraints involving these expressions will not error.
match execute_circuit(backend, program.circuit, WitnessMap::new()) {
match execute_circuit(backend, program.circuit, WitnessMap::new(), show_output) {
Ok(_) => Ok(()),
Err(error) => {
let writer = StandardStream::stderr(ColorChoice::Always);
Expand Down
20 changes: 17 additions & 3 deletions crates/nargo_cli/tests/test_data/strings/src/main.nr
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,30 @@ fn test_prints_strings() {
fn test_prints_array() {
let array = [1, 2, 3, 5, 8];

// TODO: Printing structs currently not supported
// let s = Test { a: 1, b: 2, c: [3, 4] };
// std::println(s);
let s = Test { a: 1, b: 2, c: [3, 4] };
std::println(s);

std::println(array);

let hash = std::hash::pedersen(array);
std::println(hash);
}

fn failed_constraint(hex_as_field: Field) {
// TODO(#2116): Note that `println` will not work if a failed constraint can be
// evaluated at compile time.
// When this method is called from a test method or with constant values
// a `Failed constraint` compile error will be caught before this `println`
// is executed as the input will be a constant.
std::println(hex_as_field);
assert(hex_as_field != 0x41);
}

#[test]
fn test_failed_constraint() {
failed_constraint(0x41);
}

struct Test {
a: Field,
b: Field,
Expand Down
8 changes: 3 additions & 5 deletions crates/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ pub fn compile_main(
}
};

let compiled_program = compile_no_check(context, true, options, main)?;
let compiled_program = compile_no_check(context, options, main)?;

if options.print_acir {
println!("Compiled ACIR for main (unoptimized):");
Expand Down Expand Up @@ -230,7 +230,7 @@ fn compile_contract(
let mut errs = Vec::new();
for function_id in &contract.functions {
let name = context.function_name(function_id).to_owned();
let function = match compile_no_check(context, true, options, *function_id) {
let function = match compile_no_check(context, options, *function_id) {
Ok(function) => function,
Err(err) => {
errs.push(err);
Expand Down Expand Up @@ -267,14 +267,12 @@ fn compile_contract(
#[allow(deprecated)]
pub fn compile_no_check(
context: &Context,
show_output: bool,
options: &CompileOptions,
main_function: FuncId,
) -> Result<CompiledProgram, FileDiagnostic> {
let program = monomorphize(main_function, &context.def_interner);

let (circuit, debug, abi) =
create_circuit(program, options.show_ssa, options.show_brillig, show_output)?;
let (circuit, debug, abi) = create_circuit(program, options.show_ssa, options.show_brillig)?;

Ok(CompiledProgram { circuit, debug, abi })
}
6 changes: 2 additions & 4 deletions crates/noirc_evaluator/src/ssa_refactor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ pub mod ssa_gen;
/// convert the final SSA into ACIR and return it.
pub(crate) fn optimize_into_acir(
program: Program,
allow_log_ops: bool,
print_ssa_passes: bool,
print_brillig_trace: bool,
) -> Result<GeneratedAcir, RuntimeError> {
Expand Down Expand Up @@ -63,7 +62,7 @@ pub(crate) fn optimize_into_acir(
.dead_instruction_elimination()
.print(print_ssa_passes, "After Dead Instruction Elimination:");
}
ssa.into_acir(brillig, abi_distinctness, allow_log_ops)
ssa.into_acir(brillig, abi_distinctness)
}

/// Compiles the Program into ACIR and applies optimizations to the arithmetic gates
Expand All @@ -74,7 +73,6 @@ pub fn create_circuit(
program: Program,
enable_ssa_logging: bool,
enable_brillig_logging: bool,
show_output: bool,
) -> Result<(Circuit, DebugInfo, Abi), RuntimeError> {
let func_sig = program.main_function_signature.clone();
let GeneratedAcir {
Expand All @@ -84,7 +82,7 @@ pub fn create_circuit(
locations,
input_witnesses,
..
} = optimize_into_acir(program, show_output, enable_ssa_logging, enable_brillig_logging)?;
} = optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?;

let abi = gen_abi(func_sig, &input_witnesses, return_witnesses.clone());
let public_abi = abi.clone().public_abi();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -827,19 +827,6 @@ impl AcirContext {
self.radix_decompose(endian, input_var, two_var, limb_count_var, result_element_type)
}

/// Prints the given `AcirVar`s as witnesses.
pub(crate) fn print(&mut self, input: Vec<AcirValue>) -> Result<(), RuntimeError> {
let input = Self::flatten_values(input);

let witnesses = vecmap(input, |acir_var| {
let var_data = &self.vars[&acir_var];
let expr = var_data.to_expression();
self.acir_ir.get_or_create_witness(&expr)
});
self.acir_ir.call_print(witnesses);
Ok(())
}

/// Flatten the given Vector of AcirValues into a single vector of only variables.
/// Each AcirValue::Array in the vector is recursively flattened, so each element
/// will flattened into the resulting Vec. E.g. flatten_values([1, [2, 3]) == [1, 2, 3].
Expand Down
37 changes: 9 additions & 28 deletions crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,9 @@ impl Ssa {
self,
brillig: Brillig,
abi_distinctness: AbiDistinctness,
allow_log_ops: bool,
) -> Result<GeneratedAcir, RuntimeError> {
let context = Context::new();
let mut generated_acir = context.convert_ssa(self, brillig, allow_log_ops)?;
let mut generated_acir = context.convert_ssa(self, brillig)?;

match abi_distinctness {
AbiDistinctness::Distinct => {
Expand Down Expand Up @@ -144,15 +143,10 @@ impl Context {
}

/// Converts SSA into ACIR
fn convert_ssa(
self,
ssa: Ssa,
brillig: Brillig,
allow_log_ops: bool,
) -> Result<GeneratedAcir, RuntimeError> {
fn convert_ssa(self, ssa: Ssa, brillig: Brillig) -> Result<GeneratedAcir, RuntimeError> {
let main_func = ssa.main();
match main_func.runtime() {
RuntimeType::Acir => self.convert_acir_main(main_func, &ssa, brillig, allow_log_ops),
RuntimeType::Acir => self.convert_acir_main(main_func, &ssa, brillig),
RuntimeType::Brillig => self.convert_brillig_main(main_func, brillig),
}
}
Expand All @@ -162,14 +156,13 @@ impl Context {
main_func: &Function,
ssa: &Ssa,
brillig: Brillig,
allow_log_ops: bool,
) -> Result<GeneratedAcir, RuntimeError> {
let dfg = &main_func.dfg;
let entry_block = &dfg[main_func.entry_block()];
let input_witness = self.convert_ssa_block_params(entry_block.parameters(), dfg)?;

for instruction_id in entry_block.instructions() {
self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig, allow_log_ops)?;
self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig)?;
}

self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?;
Expand Down Expand Up @@ -294,7 +287,6 @@ impl Context {
dfg: &DataFlowGraph,
ssa: &Ssa,
brillig: &Brillig,
allow_log_ops: bool,
) -> Result<(), RuntimeError> {
let instruction = &dfg[instruction_id];
self.acir_context.set_location(dfg.get_location(&instruction_id));
Expand Down Expand Up @@ -339,13 +331,8 @@ impl Context {
}
}
Value::Intrinsic(intrinsic) => {
let outputs = self.convert_ssa_intrinsic_call(
*intrinsic,
arguments,
dfg,
allow_log_ops,
result_ids,
)?;
let outputs = self
.convert_ssa_intrinsic_call(*intrinsic, arguments, dfg, result_ids)?;

// Issue #1438 causes this check to fail with intrinsics that return 0
// results but the ssa form instead creates 1 unit result value.
Expand Down Expand Up @@ -929,7 +916,6 @@ impl Context {
intrinsic: Intrinsic,
arguments: &[ValueId],
dfg: &DataFlowGraph,
allow_log_ops: bool,
result_ids: &[ValueId],
) -> Result<Vec<AcirValue>, RuntimeError> {
match intrinsic {
Expand Down Expand Up @@ -959,13 +945,8 @@ impl Context {

self.acir_context.bit_decompose(endian, field, bit_size, result_type)
}
Intrinsic::Println => {
let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg));
if allow_log_ops {
self.acir_context.print(inputs)?;
}
Ok(Vec::new())
}
// TODO(#2115): Remove the println intrinsic as the oracle println is now used instead
Intrinsic::Println => Ok(Vec::new()),
Intrinsic::Sort => {
let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg));
// We flatten the inputs and retrieve the bit_size of the elements
Expand Down Expand Up @@ -1133,7 +1114,7 @@ mod tests {
let ssa = builder.finish();

let context = Context::new();
let acir = context.convert_ssa(ssa, Brillig::default(), false).unwrap();
let acir = context.convert_ssa(ssa, Brillig::default()).unwrap();

let expected_opcodes =
vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))];
Expand Down
4 changes: 2 additions & 2 deletions crates/wasm/src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ pub fn compile(args: JsValue) -> JsValue {
<JsValue as JsValueSerdeExt>::from_serde(&optimized_contracts).unwrap()
} else {
let main = context.get_main_function(&crate_id).expect("Could not find main function!");
let mut compiled_program = compile_no_check(&context, true, &options.compile_options, main)
.expect("Compilation failed");
let mut compiled_program =
compile_no_check(&context, &options.compile_options, main).expect("Compilation failed");

compiled_program.circuit = optimize_circuit(compiled_program.circuit);

Expand Down

0 comments on commit 27ab78f

Please sign in to comment.