From 4c421adf1cea1a829b0c5ceb8c018f2e2f750ace Mon Sep 17 00:00:00 2001 From: Rumata888 Date: Mon, 1 Jul 2024 17:09:15 +0000 Subject: [PATCH 1/7] PoC --- compiler/noirc_errors/src/reporter.rs | 26 +- compiler/noirc_evaluator/src/errors.rs | 20 ++ compiler/noirc_evaluator/src/ssa.rs | 2 +- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 3 + compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 4 + .../ssa/opt/independent_subgraph_detection.rs | 306 ++++++++++++++++++ compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 + .../src/ssa/ssa_gen/program.rs | 11 +- tooling/lsp/src/notifications/mod.rs | 1 + 9 files changed, 368 insertions(+), 6 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs diff --git a/compiler/noirc_errors/src/reporter.rs b/compiler/noirc_errors/src/reporter.rs index cb5abbe2079..516d643e226 100644 --- a/compiler/noirc_errors/src/reporter.rs +++ b/compiler/noirc_errors/src/reporter.rs @@ -15,6 +15,7 @@ pub struct CustomDiagnostic { #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum DiagnosticKind { Error, + Bug, Warning, } @@ -60,6 +61,19 @@ impl CustomDiagnostic { } } + pub fn simple_bug( + primary_message: String, + secondary_message: String, + secondary_span: Span, + ) -> CustomDiagnostic { + CustomDiagnostic { + message: primary_message, + secondaries: vec![CustomLabel::new(secondary_message, secondary_span)], + notes: Vec::new(), + kind: DiagnosticKind::Bug, + } + } + pub fn in_file(self, file_id: fm::FileId) -> FileDiagnostic { FileDiagnostic::new(file_id, self) } @@ -79,6 +93,10 @@ impl CustomDiagnostic { pub fn is_warning(&self) -> bool { matches!(self.kind, DiagnosticKind::Warning) } + + pub fn is_bug(&self) -> bool { + matches!(self.kind, DiagnosticKind::Bug) + } } impl std::fmt::Display for CustomDiagnostic { @@ -118,10 +136,13 @@ pub fn report_all<'files>( silence_warnings: bool, ) -> ReportedErrors { // Report warnings before any errors - let (warnings, mut errors): (Vec<_>, _) = - diagnostics.iter().partition(|item| item.diagnostic.is_warning()); + let (warnings_and_bugs, mut errors): (Vec<_>, _) = + diagnostics.iter().partition(|item| !item.diagnostic.is_error()); + let (warnings, mut bugs): (Vec<_>, _) = + warnings_and_bugs.iter().partition(|item| item.diagnostic.is_warning()); let mut diagnostics = if silence_warnings { Vec::new() } else { warnings }; + diagnostics.append(&mut bugs); diagnostics.append(&mut errors); let error_count = @@ -166,6 +187,7 @@ fn convert_diagnostic( ) -> Diagnostic { let diagnostic = match (cd.kind, deny_warnings) { (DiagnosticKind::Warning, false) => Diagnostic::warning(), + (DiagnosticKind::Bug, ..) => Diagnostic::bug(), _ => Diagnostic::error(), }; diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index dd63a254a18..ccd2490f60f 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -50,6 +50,7 @@ pub enum RuntimeError { #[derive(Debug, Clone, Serialize, Deserialize)] pub enum SsaReport { Warning(InternalWarning), + Bug(InternalBug), } impl From for FileDiagnostic { @@ -72,6 +73,19 @@ impl From for FileDiagnostic { Diagnostic::simple_warning(message, secondary_message, location.span); diagnostic.in_file(file_id).with_call_stack(call_stack) } + SsaReport::Bug(bug) => { + let message = bug.to_string(); + let (secondary_message, call_stack) = match bug { + InternalBug::IndependentSubgraph { call_stack } => { + ("There is no path from the output of this brillig call to either return values or inputs of the circuit, which creates an independent subgraph. This is quite likely a soundness vulnerability".to_string(),call_stack) + } + }; + let call_stack = vecmap(call_stack, |location| location); + let file_id = call_stack.last().map(|location| location.file).unwrap_or_default(); + let location = call_stack.last().expect("Expected RuntimeError to have a location"); + let diagnostic = Diagnostic::simple_bug(message, secondary_message, location.span); + diagnostic.in_file(file_id).with_call_stack(call_stack) + } } } } @@ -84,6 +98,12 @@ pub enum InternalWarning { VerifyProof { call_stack: CallStack }, } +#[derive(Debug, PartialEq, Eq, Clone, Error, Serialize, Deserialize)] +pub enum InternalBug { + #[error("Input to brillig function is in a separate subgraph to output")] + IndependentSubgraph { call_stack: CallStack }, +} + #[derive(Debug, PartialEq, Eq, Clone, Error)] pub enum InternalError { #[error("ICE: Both expressions should have degree<=1")] diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 71fb51bf750..72cb16f2be9 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -84,8 +84,8 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:") .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") + .run_pass(Ssa::detect_independent_subgraphs, "After independent subgraph detection:") .finish(); - let brillig = time("SSA to Brillig", options.print_codegen_timings, || { ssa.to_brillig(options.enable_brillig_logging) }); diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index aaa483b91c9..3c26e72f58d 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -431,6 +431,9 @@ impl<'a> Context<'a> { } warnings.extend(return_warnings); + + // Add the warnings from the alter Ssa passes + warnings.extend_from_slice(&ssa.warnings); Ok(self.acir_context.finish(input_witness, return_witnesses, warnings)) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index f763ae52d50..7bf52d007eb 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -129,6 +129,10 @@ impl DataFlowGraph { self.values.iter() } + /// Retrieve the actual Value from ValueId + pub(crate) fn get_value(&self, value_id: ValueId) -> &Value { + &self.values[value_id] + } /// Returns the parameters of the given block pub(crate) fn block_parameters(&self, block: BasicBlockId) -> &[ValueId] { self.blocks[block].parameters() diff --git a/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs b/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs new file mode 100644 index 00000000000..3417f6a2af0 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs @@ -0,0 +1,306 @@ +use im::HashMap; + +use crate::errors::{InternalBug, SsaReport}; +use crate::ssa::ir::basic_block::BasicBlockId; +use crate::ssa::ir::function::RuntimeType; +use crate::ssa::ir::function::{Function, FunctionId}; +use crate::ssa::ir::instruction::{Instruction, InstructionId, Intrinsic}; +use crate::ssa::ir::value::{Value, ValueId}; +use crate::ssa::ssa_gen::Ssa; +use std::collections::{BTreeMap, HashSet}; + +impl Ssa { + #[tracing::instrument(level = "trace", skip(self))] + pub(crate) fn detect_independent_subgraphs(mut self) -> Ssa { + for function in self.functions.values() { + match function.runtime() { + RuntimeType::Acir { .. } => { + let warnings = self.detect_independent_subgraphs_within_function(function); + self.warnings.extend(warnings); + } + RuntimeType::Brillig => (), + } + } + self + } + fn detect_independent_subgraphs_within_function(&self, function: &Function) -> Vec { + let mut context = Context::default(); + let function_parameters = function.parameters(); + let mut nonconstant_parameters_and_return_values: HashSet = function_parameters + .iter() + .filter(|&x| match function.dfg[*x] { + Value::NumericConstant { .. } => false, + _ => true, + }) + .copied() + .collect(); + nonconstant_parameters_and_return_values.extend(function.returns()); + context.block_queue.push(function.entry_block()); + while let Some(block) = context.block_queue.pop() { + if context.visited_blocks.contains(&block) { + continue; + } + context.visited_blocks.insert(block); + context.connect_value_ids_in_block(function, block, &self.functions); + } + context.merge_sets(); + let mut connected_sets_indices: HashSet = HashSet::new(); + for parameter_or_return_value in nonconstant_parameters_and_return_values.iter() { + for (set_index, final_set) in context.value_sets.iter().enumerate() { + if final_set.contains(parameter_or_return_value) { + connected_sets_indices.insert(set_index); + } + } + } + let disconnected_sets_indices: Vec = + HashSet::from_iter(0..(context.value_sets.len())) + .difference(&connected_sets_indices) + .copied() + .collect(); + let all_brillig_generated_values: HashSet = + context.brillig_return_to_argument_map.keys().copied().collect(); + for set_index in disconnected_sets_indices.iter() { + let current_set = &context.value_sets[*set_index]; + let intersection: Vec = + all_brillig_generated_values.intersection(current_set).cloned().collect(); + if intersection.len() == 0 { + panic!("Somehow produced a disconnected subgraph without any brillig. How is that possible?") + } + for brillig_output_in_set in intersection.iter() { + let inputs: HashSet = HashSet::from_iter( + context.brillig_return_to_argument_map[brillig_output_in_set].iter().copied(), + ); + let unused_inputs: Vec = inputs.difference(current_set).copied().collect(); + if unused_inputs.len() != 0 { + context.warnings.push(SsaReport::Bug(InternalBug::IndependentSubgraph { + call_stack: function.dfg.get_call_stack( + context.brillig_return_to_instruction_id_map[brillig_output_in_set], + ), + })); + } + } + } + context.warnings + } +} + +#[derive(Default)] +struct Context { + visited_blocks: HashSet, + block_queue: Vec, + warnings: Vec, + value_sets: Vec>, + brillig_return_to_argument_map: HashMap>, + brillig_return_to_instruction_id_map: HashMap, +} + +impl Context { + fn connect_value_ids_in_block( + &mut self, + function: &Function, + block: BasicBlockId, + all_functions: &BTreeMap, + ) { + let instructions = function.dfg[block].instructions(); + + for instruction in instructions.iter() { + let results = function.dfg.instruction_results(*instruction); + match &function.dfg[*instruction] { + Instruction::Binary(binary) => { + let mut value_ids = vec![binary.lhs, binary.rhs]; + value_ids.extend_from_slice(results); + self.connect_values(function, &value_ids); + } + Instruction::Cast(value_id, ..) + | Instruction::Truncate { value: value_id, .. } + | Instruction::Not(value_id) => { + let mut value_ids = vec![*value_id]; + value_ids.extend_from_slice(results); + self.connect_values(function, &value_ids); + } + Instruction::Constrain(value_id1, value_id2, ..) => { + let value_ids = &[*value_id1, *value_id2]; + self.connect_values(function, value_ids); + } + Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { + let mut value_ids = + vec![*then_condition, *then_value, *else_condition, *else_value]; + + value_ids.extend_from_slice(results); + self.connect_values(function, &value_ids); + } + Instruction::Load { address } => { + let mut value_ids = vec![*address]; + value_ids.extend_from_slice(results); + self.connect_values(function, &value_ids); + } + Instruction::Store { address, value } => { + self.connect_values(function, &[*address, *value]) + } + Instruction::ArrayGet { array, index } => { + let mut value_ids = vec![*array, *index]; + value_ids.extend_from_slice(results); + self.connect_values(function, &value_ids); + } + Instruction::ArraySet { array, index, value, .. } => { + self.connect_values(function, &[*array, *index, *value]) + } + + Instruction::Call { func: func_id, arguments: argument_ids } => { + match &function.dfg[*func_id] { + Value::Intrinsic(intrinsic) => match intrinsic { + Intrinsic::IsUnconstrained + | Intrinsic::AsWitness + | Intrinsic::ApplyRangeConstraint + | Intrinsic::AssertConstant => {} + _ => { + let mut value_ids = argument_ids.clone(); + value_ids.extend_from_slice(results); + self.connect_values(function, &value_ids); + } + }, + Value::Function(callee) => match all_functions[&callee].runtime() { + RuntimeType::Brillig => { + for result in results.iter() { + self.brillig_return_to_argument_map + .insert(*result, argument_ids.clone()); + self.brillig_return_to_instruction_id_map + .insert(*result, instruction.clone()); + } + } + _ => { + let mut value_ids = argument_ids.clone(); + value_ids.extend_from_slice(results); + self.connect_values(function, &value_ids); + } + }, + Value::ForeignFunction(..) => { + panic!("Should not be able to reach foreign function from non-brillig functions"); + } + _ => { + panic!("At the point we are running disconnect there shouldn't be any other values as arguments") + } + } + } + _ => {} + } + } + + self.block_queue.extend(function.dfg[block].successors()); + } + + fn connect_values(&mut self, function: &Function, values: &[ValueId]) { + self.value_sets.push(HashSet::from_iter( + values + .iter() + .filter(|value_id| match function.dfg.get_value(**value_id) { + Value::NumericConstant { .. } => false, + _ => true, + }) + .cloned(), + )); + } + + fn merge_sets(&mut self) { + let mut new_set_id: usize = 0; + let mut updated_sets: HashMap> = HashMap::new(); + let mut value_dictionary: HashMap = HashMap::new(); + let mut parse_value_set: HashSet = HashSet::new(); + for set in self.value_sets.iter() { + let intersection: HashSet = + set.intersection(&parse_value_set).cloned().collect(); + parse_value_set.extend(set.iter()); + if intersection.is_empty() { + updated_sets.insert(new_set_id, set.clone()); + for entry in set.iter() { + value_dictionary.insert(*entry, new_set_id); + } + new_set_id += 1; + continue; + } + let mut joining_sets_ids: HashSet = + intersection.iter().map(|x| value_dictionary[x]).collect(); + let mut largest_set_size = usize::MIN; + let mut largest_set_index = usize::MAX; + for set_id in joining_sets_ids.iter() { + if updated_sets[set_id].len() > largest_set_size { + (largest_set_index, largest_set_size) = (*set_id, updated_sets[set_id].len()); + } + } + joining_sets_ids.remove(&largest_set_index); + + let mut largest_set = + updated_sets.extract(&largest_set_index).expect("Set should be in the hashmap").0; + for set_id in joining_sets_ids.iter() { + for element in updated_sets[set_id].iter() { + value_dictionary[element] = largest_set_index; + largest_set.insert(*element); + } + updated_sets.remove(set_id); + } + for element in set.iter() { + value_dictionary.insert(*element, largest_set_index); + largest_set.insert(*element); + } + updated_sets.insert(largest_set_index, largest_set); + } + self.value_sets = updated_sets.values().cloned().collect(); + } +} +#[cfg(test)] +mod test { + use crate::ssa::{ + function_builder::FunctionBuilder, + ir::{instruction::BinaryOp, map::Id, types::Type}, + }; + + #[test] + fn test_simple_connected_function() { + let main_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), main_id); + let v0 = builder.add_parameter(Type::field()); + let v1 = builder.add_parameter(Type::field()); + + let one = builder.field_constant(1u128); + let two = builder.field_constant(2u128); + + let v2 = builder.insert_binary(v0, BinaryOp::Add, one); + let v3 = builder.insert_binary(v1, BinaryOp::Mul, two); + let _v4 = builder.insert_binary(v2, BinaryOp::Eq, v3); + builder.terminate_with_return(vec![v2]); + + let mut ssa = builder.finish(); + ssa = ssa.detect_independent_subgraphs(); + assert_eq!(ssa.warnings.len(), 0); + } + + #[test] + fn test_simple_function_with_disconnected_part() { + let main_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), main_id); + let v0 = builder.add_parameter(Type::field()); + let v1 = builder.add_parameter(Type::field()); + + let one = builder.field_constant(1u128); + let two = builder.field_constant(2u128); + + let v2 = builder.insert_binary(v0, BinaryOp::Add, one); + let v3 = builder.insert_binary(v1, BinaryOp::Mul, two); + + let br_function_id = Id::test_new(1); + let br_function = builder.import_function(br_function_id); + let v4 = builder.insert_call(br_function, vec![v2, v3], vec![Type::field()])[0]; + let v5 = builder.insert_binary(v4, BinaryOp::Add, two); + builder.insert_constrain(v5, one, None); + builder.terminate_with_return(vec![]); + + builder.new_brillig_function("br".into(), br_function_id); + let v0 = builder.add_parameter(Type::field()); + let v1 = builder.add_parameter(Type::field()); + let v2 = builder.insert_binary(v0, BinaryOp::Add, v1); + builder.terminate_with_return(vec![v2]); + let mut ssa = builder.finish(); + ssa = ssa.detect_independent_subgraphs(); + assert_eq!(ssa.warnings.len(), 1); + } +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 4e5fa262696..27a5e37b20a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -11,6 +11,7 @@ mod constant_folding; mod defunctionalize; mod die; pub(crate) mod flatten_cfg; +mod independent_subgraph_detection; mod inlining; mod mem2reg; mod rc; diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 7a77aa76101..6798eb8196b 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -3,9 +3,12 @@ use std::{collections::BTreeMap, fmt::Display}; use acvm::acir::circuit::ErrorSelector; use iter_extended::btree_map; -use crate::ssa::ir::{ - function::{Function, FunctionId, RuntimeType}, - map::AtomicCounter, +use crate::{ + errors::SsaReport, + ssa::ir::{ + function::{Function, FunctionId, RuntimeType}, + map::AtomicCounter, + }, }; use noirc_frontend::hir_def::types::Type as HirType; @@ -20,6 +23,7 @@ pub(crate) struct Ssa { /// as the final program artifact will be a list of only entry point functions. pub(crate) entry_point_to_generated_index: BTreeMap, pub(crate) error_selector_to_type: BTreeMap, + pub(crate) warnings: Vec, } impl Ssa { @@ -57,6 +61,7 @@ impl Ssa { next_id: AtomicCounter::starting_after(max_id), entry_point_to_generated_index, error_selector_to_type: error_types, + warnings: Vec::new(), } } diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 3856bdc79e9..6c9bb0801a2 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -190,6 +190,7 @@ fn process_noir_document( let severity = match diagnostic.kind { DiagnosticKind::Error => DiagnosticSeverity::ERROR, DiagnosticKind::Warning => DiagnosticSeverity::WARNING, + DiagnosticKind::Bug => DiagnosticSeverity::WARNING, }; Some(Diagnostic { range, From c60ad3635445479235ab34deed0cd6c66539ddba Mon Sep 17 00:00:00 2001 From: Rumata888 Date: Wed, 3 Jul 2024 16:22:39 +0000 Subject: [PATCH 2/7] comments --- .../ssa/opt/independent_subgraph_detection.rs | 112 +++++++++++++++--- 1 file changed, 95 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs b/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs index 3417f6a2af0..7543c424c13 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs @@ -1,3 +1,6 @@ +//! This module defines an SSA pass that detects if the final function has any subgraphs independent from inputs and outputs. +//! If this is the case, then part of the final circuit can be completely replaced by any other passing circuit, since there are no constraints ensuring connections. +//! So the compiler informs the developer of this as a bug use im::HashMap; use crate::errors::{InternalBug, SsaReport}; @@ -10,6 +13,7 @@ use crate::ssa::ssa_gen::Ssa; use std::collections::{BTreeMap, HashSet}; impl Ssa { + /// Go through each top-level non-brillig function and detect if it has independent subgraphs #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn detect_independent_subgraphs(mut self) -> Ssa { for function in self.functions.values() { @@ -23,18 +27,12 @@ impl Ssa { } self } + + /// Detect independent subgraphs (not connected to function inputs or outputs) and return a vector of bug reports if some are found fn detect_independent_subgraphs_within_function(&self, function: &Function) -> Vec { let mut context = Context::default(); - let function_parameters = function.parameters(); - let mut nonconstant_parameters_and_return_values: HashSet = function_parameters - .iter() - .filter(|&x| match function.dfg[*x] { - Value::NumericConstant { .. } => false, - _ => true, - }) - .copied() - .collect(); - nonconstant_parameters_and_return_values.extend(function.returns()); + + // Go through each block in the function and create a list of sets of ValueIds connected by instructions context.block_queue.push(function.entry_block()); while let Some(block) = context.block_queue.pop() { if context.visited_blocks.contains(&block) { @@ -43,35 +41,67 @@ impl Ssa { context.visited_blocks.insert(block); context.connect_value_ids_in_block(function, block, &self.functions); } + + // Merge ValueIds into sets, where each original small set of ValueIds is merged with another set if they intersect context.merge_sets(); + + let function_parameters = function.parameters(); + let variable_parameters_and_return_values: HashSet = function_parameters + .iter() + .chain(function.returns()) + .filter(|&x| match function.dfg[*x] { + Value::NumericConstant { .. } => false, // Constant values don't connect elements and can be reused in different subgraphs, so we need to avoid them + _ => true, + }) + .copied() + .collect(); + let mut connected_sets_indices: HashSet = HashSet::new(); - for parameter_or_return_value in nonconstant_parameters_and_return_values.iter() { + + // Go through each parameter and each set and check if the set contains the parameter + // If it's the case, then that set doesn't present an issue + for parameter_or_return_value in variable_parameters_and_return_values.iter() { for (set_index, final_set) in context.value_sets.iter().enumerate() { if final_set.contains(parameter_or_return_value) { connected_sets_indices.insert(set_index); } } } + + // All the other sets of variables are independent let disconnected_sets_indices: Vec = HashSet::from_iter(0..(context.value_sets.len())) .difference(&connected_sets_indices) .copied() .collect(); + let all_brillig_generated_values: HashSet = context.brillig_return_to_argument_map.keys().copied().collect(); + + // Go through each disconnected set for set_index in disconnected_sets_indices.iter() { let current_set = &context.value_sets[*set_index]; + + // Find brillig-generated values in the set let intersection: Vec = all_brillig_generated_values.intersection(current_set).cloned().collect(); - if intersection.len() == 0 { - panic!("Somehow produced a disconnected subgraph without any brillig. How is that possible?") + if intersection.is_empty() { + // This is probably a test and the values are optimized in a weird way + continue; } + + // Go through all brillig outputs in the set for brillig_output_in_set in intersection.iter() { + // Get the inputs that correspond to the output let inputs: HashSet = HashSet::from_iter( context.brillig_return_to_argument_map[brillig_output_in_set].iter().copied(), ); + + // Check if any of them are not in the set let unused_inputs: Vec = inputs.difference(current_set).copied().collect(); - if unused_inputs.len() != 0 { + + // There is a value not in the set, which means that the inputs/outputs of this call have not been properly constrained + if !unused_inputs.is_empty() { context.warnings.push(SsaReport::Bug(InternalBug::IndependentSubgraph { call_stack: function.dfg.get_call_stack( context.brillig_return_to_instruction_id_map[brillig_output_in_set], @@ -95,6 +125,7 @@ struct Context { } impl Context { + /// Go through each instruction in the block and add a set of ValueIds connected through that instruction fn connect_value_ids_in_block( &mut self, function: &Function, @@ -105,6 +136,7 @@ impl Context { for instruction in instructions.iter() { let results = function.dfg.instruction_results(*instruction); + // For most instructions we just connect inputs and outputs match &function.dfg[*instruction] { Instruction::Binary(binary) => { let mut value_ids = vec![binary.lhs, binary.rhs]; @@ -161,6 +193,8 @@ impl Context { }, Value::Function(callee) => match all_functions[&callee].runtime() { RuntimeType::Brillig => { + // For calls to brillig functions we memorize the mapping of results to argument ValueId's and InstructionId's + // The latter are needed to produce the callstack later for result in results.iter() { self.brillig_return_to_argument_map .insert(*result, argument_ids.clone()); @@ -189,6 +223,7 @@ impl Context { self.block_queue.extend(function.dfg[block].successors()); } + /// Add a set of ValueIds to the vector of connected values while ignoring constants fn connect_values(&mut self, function: &Function, values: &[ValueId]) { self.value_sets.push(HashSet::from_iter( values @@ -201,27 +236,41 @@ impl Context { )); } + /// Merge all small sets into larger ones based on whether the sets intersect or not + /// + /// If two small sets have a common ValueId, we merge them into one fn merge_sets(&mut self) { let mut new_set_id: usize = 0; let mut updated_sets: HashMap> = HashMap::new(); let mut value_dictionary: HashMap = HashMap::new(); - let mut parse_value_set: HashSet = HashSet::new(); + let mut parsed_value_set: HashSet = HashSet::new(); + + // Go through each set for set in self.value_sets.iter() { + // Check if the set has any of the ValueIds we've encountered at previous iterations let intersection: HashSet = - set.intersection(&parse_value_set).cloned().collect(); - parse_value_set.extend(set.iter()); + set.intersection(&parsed_value_set).cloned().collect(); + parsed_value_set.extend(set.iter()); + + // If there is no intersection, add the new set to updated sets if intersection.is_empty() { updated_sets.insert(new_set_id, set.clone()); + + // Add each entry to a dictionary for quick lookups of which ValueId is in which updated set for entry in set.iter() { value_dictionary.insert(*entry, new_set_id); } new_set_id += 1; continue; } + + // If there is an intersection, we have to join the sets let mut joining_sets_ids: HashSet = intersection.iter().map(|x| value_dictionary[x]).collect(); let mut largest_set_size = usize::MIN; let mut largest_set_index = usize::MAX; + + // We need to find the largest set to move as few elements as possible for set_id in joining_sets_ids.iter() { if updated_sets[set_id].len() > largest_set_size { (largest_set_index, largest_set_size) = (*set_id, updated_sets[set_id].len()); @@ -231,13 +280,19 @@ impl Context { let mut largest_set = updated_sets.extract(&largest_set_index).expect("Set should be in the hashmap").0; + + // For each of other sets that need to be joined for set_id in joining_sets_ids.iter() { + // Map each element to the largest set and insert it for element in updated_sets[set_id].iter() { value_dictionary[element] = largest_set_index; largest_set.insert(*element); } + // Remove the old set updated_sets.remove(set_id); } + + // Join the new set with the largest sets for element in set.iter() { value_dictionary.insert(*element, largest_set_index); largest_set.insert(*element); @@ -255,7 +310,15 @@ mod test { }; #[test] + /// Test that a connected function raises no warnings fn test_simple_connected_function() { + // fn main { + // b0(v0: Field, v1: Field): + // v2 = add v0, 1 + // v3 = mul v1, 2 + // v4 = eq v2, v3 + // return v2 + // } let main_id = Id::test_new(0); let mut builder = FunctionBuilder::new("main".into(), main_id); let v0 = builder.add_parameter(Type::field()); @@ -275,7 +338,22 @@ mod test { } #[test] + /// Test where the results of a call to a brillig function are not connected to main function inputs or outputs + /// This should be detected. fn test_simple_function_with_disconnected_part() { + // unconstrained fn br(v0: Field, v1: Field){ + // v2 = add v0, v1 + // return v2 + // } + // + // fn main { + // b0(v0: Field, v1: Field): + // v2 = add v0, 1 + // v3 = mul v1, 2 + // v4 = call br(v2, v3) + // v5 = add v4, 2 + // return + // } let main_id = Id::test_new(0); let mut builder = FunctionBuilder::new("main".into(), main_id); let v0 = builder.add_parameter(Type::field()); From c338fc0debca48c9573d34b57d09f6ce8fb92008 Mon Sep 17 00:00:00 2001 From: Rumata888 Date: Wed, 3 Jul 2024 16:53:53 +0000 Subject: [PATCH 3/7] formatting --- .../src/ssa/opt/independent_subgraph_detection.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs b/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs index 7543c424c13..736aa9d0fa3 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs @@ -167,7 +167,7 @@ impl Context { self.connect_values(function, &value_ids); } Instruction::Store { address, value } => { - self.connect_values(function, &[*address, *value]) + self.connect_values(function, &[*address, *value]); } Instruction::ArrayGet { array, index } => { let mut value_ids = vec![*array, *index]; @@ -175,7 +175,7 @@ impl Context { self.connect_values(function, &value_ids); } Instruction::ArraySet { array, index, value, .. } => { - self.connect_values(function, &[*array, *index, *value]) + self.connect_values(function, &[*array, *index, *value]); } Instruction::Call { func: func_id, arguments: argument_ids } => { @@ -199,7 +199,7 @@ impl Context { self.brillig_return_to_argument_map .insert(*result, argument_ids.clone()); self.brillig_return_to_instruction_id_map - .insert(*result, instruction.clone()); + .insert(*result, *instruction); } } _ => { @@ -228,9 +228,8 @@ impl Context { self.value_sets.push(HashSet::from_iter( values .iter() - .filter(|value_id| match function.dfg.get_value(**value_id) { - Value::NumericConstant { .. } => false, - _ => true, + .filter(|value_id| { + !matches!(function.dfg.get_value(**value_id), Value::NumericConstant { .. }) }) .cloned(), )); From 79291adddb7ada200f5063a02351906381bcda78 Mon Sep 17 00:00:00 2001 From: Rumata888 Date: Thu, 4 Jul 2024 23:29:25 +0000 Subject: [PATCH 4/7] Address most Jake's comments (function splitting left) --- compiler/noirc_evaluator/src/ssa.rs | 22 +- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 1 - compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 4 - ...s => check_for_underconstrained_values.rs} | 296 +++++++++--------- compiler/noirc_evaluator/src/ssa/opt/mod.rs | 2 +- .../src/ssa/ssa_gen/program.rs | 11 +- 6 files changed, 164 insertions(+), 172 deletions(-) rename compiler/noirc_evaluator/src/ssa/opt/{independent_subgraph_detection.rs => check_for_underconstrained_values.rs} (53%) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index b4acc289370..e1182f17bcd 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -41,6 +41,8 @@ pub mod ir; mod opt; pub mod ssa_gen; +pub(crate) struct ArtifactsAndWarnings(Artifacts, Vec); + /// Optimize the given program by converting it into SSA /// form and performing optimizations there. When finished, /// convert the final SSA into an ACIR program and return it. @@ -49,10 +51,10 @@ pub mod ssa_gen; pub(crate) fn optimize_into_acir( program: Program, options: &SsaEvaluatorOptions, -) -> Result { +) -> Result { let ssa_gen_span = span!(Level::TRACE, "ssa_generation"); let ssa_gen_span_guard = ssa_gen_span.enter(); - let ssa = SsaBuilder::new( + let mut ssa = SsaBuilder::new( program, options.enable_ssa_logging, options.force_brillig_output, @@ -87,15 +89,17 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:") .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") - .run_pass(Ssa::detect_independent_subgraphs, "After independent subgraph detection:") .finish(); + + let ssa_level_warnings = ssa.check_for_underconstrained_values(); let brillig = time("SSA to Brillig", options.print_codegen_timings, || { ssa.to_brillig(options.enable_brillig_logging) }); drop(ssa_gen_span_guard); - time("SSA to ACIR", options.print_codegen_timings, || ssa.into_acir(&brillig)) + let artifacts = time("SSA to ACIR", options.print_codegen_timings, || ssa.into_acir(&brillig))?; + Ok(ArtifactsAndWarnings(artifacts, ssa_level_warnings)) } // Helper to time SSA passes @@ -149,6 +153,10 @@ impl SsaProgramArtifact { } self.names.push(circuit_artifact.name); } + + fn add_warnings(&mut self, mut warnings: Vec) { + self.warnings.append(&mut warnings); + } } pub struct SsaEvaluatorOptions { @@ -179,7 +187,8 @@ pub fn create_program( let func_sigs = program.function_signatures.clone(); let recursive = program.recursive; - let (generated_acirs, generated_brillig, error_types) = optimize_into_acir(program, options)?; + let ArtifactsAndWarnings((generated_acirs, generated_brillig, error_types), ssa_level_warnings) = + optimize_into_acir(program, options)?; assert_eq!( generated_acirs.len(), func_sigs.len(), @@ -187,6 +196,9 @@ pub fn create_program( ); let mut program_artifact = SsaProgramArtifact::new(generated_brillig, error_types); + + // Add warnings collected at the Ssa stage + program_artifact.add_warnings(ssa_level_warnings); // For setting up the ABI we need separately specify main's input and return witnesses let mut is_main = true; for (acir, func_sig) in generated_acirs.into_iter().zip(func_sigs) { diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 3c26e72f58d..3b7d2c1025f 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -433,7 +433,6 @@ impl<'a> Context<'a> { warnings.extend(return_warnings); // Add the warnings from the alter Ssa passes - warnings.extend_from_slice(&ssa.warnings); Ok(self.acir_context.finish(input_witness, return_witnesses, warnings)) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index f089cac04f9..994386f8197 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -129,10 +129,6 @@ impl DataFlowGraph { self.values.iter() } - /// Retrieve the actual Value from ValueId - pub(crate) fn get_value(&self, value_id: ValueId) -> &Value { - &self.values[value_id] - } /// Returns the parameters of the given block pub(crate) fn block_parameters(&self, block: BasicBlockId) -> &[ValueId] { self.blocks[block].parameters() diff --git a/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs b/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs similarity index 53% rename from compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs rename to compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs index 736aa9d0fa3..dfd59d7d51a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/independent_subgraph_detection.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs @@ -15,113 +15,104 @@ use std::collections::{BTreeMap, HashSet}; impl Ssa { /// Go through each top-level non-brillig function and detect if it has independent subgraphs #[tracing::instrument(level = "trace", skip(self))] - pub(crate) fn detect_independent_subgraphs(mut self) -> Ssa { + pub(crate) fn check_for_underconstrained_values(&mut self) -> Vec { + let mut warnings: Vec = Vec::new(); for function in self.functions.values() { match function.runtime() { RuntimeType::Acir { .. } => { - let warnings = self.detect_independent_subgraphs_within_function(function); - self.warnings.extend(warnings); + warnings.extend(check_for_underconstrained_values_within_function( + function, + &self.functions, + )); } RuntimeType::Brillig => (), } } - self + warnings } +} - /// Detect independent subgraphs (not connected to function inputs or outputs) and return a vector of bug reports if some are found - fn detect_independent_subgraphs_within_function(&self, function: &Function) -> Vec { - let mut context = Context::default(); - - // Go through each block in the function and create a list of sets of ValueIds connected by instructions - context.block_queue.push(function.entry_block()); - while let Some(block) = context.block_queue.pop() { - if context.visited_blocks.contains(&block) { - continue; - } - context.visited_blocks.insert(block); - context.connect_value_ids_in_block(function, block, &self.functions); - } - - // Merge ValueIds into sets, where each original small set of ValueIds is merged with another set if they intersect - context.merge_sets(); - - let function_parameters = function.parameters(); - let variable_parameters_and_return_values: HashSet = function_parameters - .iter() - .chain(function.returns()) - .filter(|&x| match function.dfg[*x] { - Value::NumericConstant { .. } => false, // Constant values don't connect elements and can be reused in different subgraphs, so we need to avoid them - _ => true, - }) - .copied() - .collect(); - - let mut connected_sets_indices: HashSet = HashSet::new(); - - // Go through each parameter and each set and check if the set contains the parameter - // If it's the case, then that set doesn't present an issue - for parameter_or_return_value in variable_parameters_and_return_values.iter() { - for (set_index, final_set) in context.value_sets.iter().enumerate() { - if final_set.contains(parameter_or_return_value) { - connected_sets_indices.insert(set_index); - } - } +/// Detect independent subgraphs (not connected to function inputs or outputs) and return a vector of bug reports if some are found +fn check_for_underconstrained_values_within_function( + function: &Function, + all_functions: &BTreeMap, +) -> Vec { + let mut context = Context::default(); + let mut warnings: Vec = Vec::new(); + + // Go through each block in the function and create a list of sets of ValueIds connected by instructions + context.block_queue.push(function.entry_block()); + while let Some(block) = context.block_queue.pop() { + if context.visited_blocks.contains(&block) { + continue; } + context.visited_blocks.insert(block); + context.connect_value_ids_in_block(function, block, all_functions); + } - // All the other sets of variables are independent - let disconnected_sets_indices: Vec = - HashSet::from_iter(0..(context.value_sets.len())) - .difference(&connected_sets_indices) - .copied() - .collect(); + // Merge ValueIds into sets, where each original small set of ValueIds is merged with another set if they intersect + context.merge_sets(); - let all_brillig_generated_values: HashSet = - context.brillig_return_to_argument_map.keys().copied().collect(); + let function_parameters = function.parameters(); + let variable_parameters_and_return_values = function_parameters + .iter() + .chain(function.returns()) + .filter(|id| function.dfg.get_numeric_constant(**id).is_none()) + .copied(); - // Go through each disconnected set - for set_index in disconnected_sets_indices.iter() { - let current_set = &context.value_sets[*set_index]; + let mut connected_sets_indices: HashSet = HashSet::new(); - // Find brillig-generated values in the set - let intersection: Vec = - all_brillig_generated_values.intersection(current_set).cloned().collect(); - if intersection.is_empty() { - // This is probably a test and the values are optimized in a weird way - continue; + // Go through each parameter and each set and check if the set contains the parameter + // If it's the case, then that set doesn't present an issue + for parameter_or_return_value in variable_parameters_and_return_values { + for (set_index, final_set) in context.value_sets.iter().enumerate() { + if final_set.contains(¶meter_or_return_value) { + connected_sets_indices.insert(set_index); } + } + } - // Go through all brillig outputs in the set - for brillig_output_in_set in intersection.iter() { - // Get the inputs that correspond to the output - let inputs: HashSet = HashSet::from_iter( - context.brillig_return_to_argument_map[brillig_output_in_set].iter().copied(), - ); - - // Check if any of them are not in the set - let unused_inputs: Vec = inputs.difference(current_set).copied().collect(); - - // There is a value not in the set, which means that the inputs/outputs of this call have not been properly constrained - if !unused_inputs.is_empty() { - context.warnings.push(SsaReport::Bug(InternalBug::IndependentSubgraph { - call_stack: function.dfg.get_call_stack( - context.brillig_return_to_instruction_id_map[brillig_output_in_set], - ), - })); - } + let all_brillig_generated_values: HashSet = + context.brillig_return_to_argument.keys().copied().collect(); + + // Go through each disconnected set + for set_index in + HashSet::from_iter(0..(context.value_sets.len())).difference(&connected_sets_indices) + { + let current_set = &context.value_sets[*set_index]; + + // Find brillig-generated values in the set + let intersection = all_brillig_generated_values.intersection(current_set).copied(); + + // Go through all brillig outputs in the set + for brillig_output_in_set in intersection { + // Get the inputs that correspond to the output + let inputs: HashSet = HashSet::from_iter( + context.brillig_return_to_argument[&brillig_output_in_set].iter().copied(), + ); + + // Check if any of them are not in the set + let unused_inputs = inputs.difference(current_set).next().is_some(); + + // There is a value not in the set, which means that the inputs/outputs of this call have not been properly constrained + if unused_inputs { + warnings.push(SsaReport::Bug(InternalBug::IndependentSubgraph { + call_stack: function.dfg.get_call_stack( + context.brillig_return_to_instruction_id[&brillig_output_in_set], + ), + })); } } - context.warnings } + warnings } - #[derive(Default)] struct Context { visited_blocks: HashSet, block_queue: Vec, - warnings: Vec, value_sets: Vec>, - brillig_return_to_argument_map: HashMap>, - brillig_return_to_instruction_id_map: HashMap, + brillig_return_to_argument: HashMap>, + brillig_return_to_instruction_id: HashMap, } impl Context { @@ -135,106 +126,105 @@ impl Context { let instructions = function.dfg[block].instructions(); for instruction in instructions.iter() { - let results = function.dfg.instruction_results(*instruction); - // For most instructions we just connect inputs and outputs - match &function.dfg[*instruction] { - Instruction::Binary(binary) => { - let mut value_ids = vec![binary.lhs, binary.rhs]; - value_ids.extend_from_slice(results); - self.connect_values(function, &value_ids); - } - Instruction::Cast(value_id, ..) - | Instruction::Truncate { value: value_id, .. } - | Instruction::Not(value_id) => { - let mut value_ids = vec![*value_id]; - value_ids.extend_from_slice(results); - self.connect_values(function, &value_ids); - } - Instruction::Constrain(value_id1, value_id2, ..) => { - let value_ids = &[*value_id1, *value_id2]; - self.connect_values(function, value_ids); - } - Instruction::IfElse { then_condition, then_value, else_condition, else_value } => { - let mut value_ids = - vec![*then_condition, *then_value, *else_condition, *else_value]; + let mut instruction_arguments_and_results = HashSet::new(); - value_ids.extend_from_slice(results); - self.connect_values(function, &value_ids); - } - Instruction::Load { address } => { - let mut value_ids = vec![*address]; - value_ids.extend_from_slice(results); - self.connect_values(function, &value_ids); + // Insert non-constant instruction arguments + function.dfg[*instruction].for_each_value(|value_id| { + if function.dfg.get_numeric_constant(value_id).is_none() { + instruction_arguments_and_results.insert(value_id); } - Instruction::Store { address, value } => { - self.connect_values(function, &[*address, *value]); + }); + // And non-constant results + for value_id in function.dfg.instruction_results(*instruction).iter() { + if function.dfg.get_numeric_constant(*value_id).is_none() { + instruction_arguments_and_results.insert(*value_id); } - Instruction::ArrayGet { array, index } => { - let mut value_ids = vec![*array, *index]; - value_ids.extend_from_slice(results); - self.connect_values(function, &value_ids); - } - Instruction::ArraySet { array, index, value, .. } => { - self.connect_values(function, &[*array, *index, *value]); + } + + // For most instructions we just connect inputs and outputs + match &function.dfg[*instruction] { + Instruction::ArrayGet { .. } + | Instruction::ArraySet { .. } + | Instruction::Binary(..) + | Instruction::Cast(..) + | Instruction::Constrain(..) + | Instruction::IfElse { .. } + | Instruction::Load { .. } + | Instruction::Not(..) + | Instruction::Store { .. } + | Instruction::Truncate { .. } => { + self.value_sets.push(instruction_arguments_and_results); } Instruction::Call { func: func_id, arguments: argument_ids } => { match &function.dfg[*func_id] { Value::Intrinsic(intrinsic) => match intrinsic { - Intrinsic::IsUnconstrained + Intrinsic::ApplyRangeConstraint + | Intrinsic::AssertConstant | Intrinsic::AsWitness - | Intrinsic::ApplyRangeConstraint - | Intrinsic::AssertConstant => {} - _ => { - let mut value_ids = argument_ids.clone(); - value_ids.extend_from_slice(results); - self.connect_values(function, &value_ids); + | Intrinsic::IsUnconstrained => {} + Intrinsic::ArrayLen + | Intrinsic::AsField + | Intrinsic::AsSlice + | Intrinsic::BlackBox(..) + | Intrinsic::DerivePedersenGenerators + | Intrinsic::FromField + | Intrinsic::SlicePushBack + | Intrinsic::SlicePushFront + | Intrinsic::SlicePopBack + | Intrinsic::SlicePopFront + | Intrinsic::SliceInsert + | Intrinsic::SliceRemove + | Intrinsic::StaticAssert + | Intrinsic::StrAsBytes + | Intrinsic::ToBits(..) + | Intrinsic::ToRadix(..) => { + self.value_sets.push(instruction_arguments_and_results); } }, Value::Function(callee) => match all_functions[&callee].runtime() { RuntimeType::Brillig => { // For calls to brillig functions we memorize the mapping of results to argument ValueId's and InstructionId's // The latter are needed to produce the callstack later - for result in results.iter() { - self.brillig_return_to_argument_map + for result in + function.dfg.instruction_results(*instruction).iter().filter( + |value_id| { + function.dfg.get_numeric_constant(**value_id).is_none() + }, + ) + { + self.brillig_return_to_argument .insert(*result, argument_ids.clone()); - self.brillig_return_to_instruction_id_map + self.brillig_return_to_instruction_id .insert(*result, *instruction); } } - _ => { - let mut value_ids = argument_ids.clone(); - value_ids.extend_from_slice(results); - self.connect_values(function, &value_ids); + RuntimeType::Acir(..) => { + self.value_sets.push(instruction_arguments_and_results); } }, Value::ForeignFunction(..) => { panic!("Should not be able to reach foreign function from non-brillig functions"); } - _ => { + Value::Array { .. } + | Value::Instruction { .. } + | Value::NumericConstant { .. } + | Value::Param { .. } => { panic!("At the point we are running disconnect there shouldn't be any other values as arguments") } } } - _ => {} + Instruction::Allocate { .. } + | Instruction::DecrementRc { .. } + | Instruction::EnableSideEffects { .. } + | Instruction::IncrementRc { .. } + | Instruction::RangeCheck { .. } => {} } } self.block_queue.extend(function.dfg[block].successors()); } - /// Add a set of ValueIds to the vector of connected values while ignoring constants - fn connect_values(&mut self, function: &Function, values: &[ValueId]) { - self.value_sets.push(HashSet::from_iter( - values - .iter() - .filter(|value_id| { - !matches!(function.dfg.get_value(**value_id), Value::NumericConstant { .. }) - }) - .cloned(), - )); - } - /// Merge all small sets into larger ones based on whether the sets intersect or not /// /// If two small sets have a common ValueId, we merge them into one @@ -248,7 +238,7 @@ impl Context { for set in self.value_sets.iter() { // Check if the set has any of the ValueIds we've encountered at previous iterations let intersection: HashSet = - set.intersection(&parsed_value_set).cloned().collect(); + set.intersection(&parsed_value_set).copied().collect(); parsed_value_set.extend(set.iter()); // If there is no intersection, add the new set to updated sets @@ -332,8 +322,8 @@ mod test { builder.terminate_with_return(vec![v2]); let mut ssa = builder.finish(); - ssa = ssa.detect_independent_subgraphs(); - assert_eq!(ssa.warnings.len(), 0); + let ssa_level_warnings = ssa.check_for_underconstrained_values(); + assert_eq!(ssa_level_warnings.len(), 0); } #[test] @@ -377,7 +367,7 @@ mod test { let v2 = builder.insert_binary(v0, BinaryOp::Add, v1); builder.terminate_with_return(vec![v2]); let mut ssa = builder.finish(); - ssa = ssa.detect_independent_subgraphs(); - assert_eq!(ssa.warnings.len(), 1); + let ssa_level_warnings = ssa.check_for_underconstrained_values(); + assert_eq!(ssa_level_warnings.len(), 1); } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 27a5e37b20a..56484ced290 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -7,11 +7,11 @@ mod array_set; mod as_slice_length; mod assert_constant; mod bubble_up_constrains; +mod check_for_underconstrained_values; mod constant_folding; mod defunctionalize; mod die; pub(crate) mod flatten_cfg; -mod independent_subgraph_detection; mod inlining; mod mem2reg; mod rc; diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 6798eb8196b..7a77aa76101 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -3,12 +3,9 @@ use std::{collections::BTreeMap, fmt::Display}; use acvm::acir::circuit::ErrorSelector; use iter_extended::btree_map; -use crate::{ - errors::SsaReport, - ssa::ir::{ - function::{Function, FunctionId, RuntimeType}, - map::AtomicCounter, - }, +use crate::ssa::ir::{ + function::{Function, FunctionId, RuntimeType}, + map::AtomicCounter, }; use noirc_frontend::hir_def::types::Type as HirType; @@ -23,7 +20,6 @@ pub(crate) struct Ssa { /// as the final program artifact will be a list of only entry point functions. pub(crate) entry_point_to_generated_index: BTreeMap, pub(crate) error_selector_to_type: BTreeMap, - pub(crate) warnings: Vec, } impl Ssa { @@ -61,7 +57,6 @@ impl Ssa { next_id: AtomicCounter::starting_after(max_id), entry_point_to_generated_index, error_selector_to_type: error_types, - warnings: Vec::new(), } } From 4da692b945ab81c5791504632fc51179f5a3f69b Mon Sep 17 00:00:00 2001 From: Rumata888 Date: Fri, 5 Jul 2024 16:20:49 +0000 Subject: [PATCH 5/7] Split function --- .../opt/check_for_underconstrained_values.rs | 131 ++++++++++++------ 1 file changed, 85 insertions(+), 46 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs index dfd59d7d51a..9345f07a980 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs @@ -40,47 +40,95 @@ fn check_for_underconstrained_values_within_function( let mut context = Context::default(); let mut warnings: Vec = Vec::new(); - // Go through each block in the function and create a list of sets of ValueIds connected by instructions - context.block_queue.push(function.entry_block()); - while let Some(block) = context.block_queue.pop() { - if context.visited_blocks.contains(&block) { - continue; - } - context.visited_blocks.insert(block); - context.connect_value_ids_in_block(function, block, all_functions); - } - - // Merge ValueIds into sets, where each original small set of ValueIds is merged with another set if they intersect - context.merge_sets(); - - let function_parameters = function.parameters(); - let variable_parameters_and_return_values = function_parameters - .iter() - .chain(function.returns()) - .filter(|id| function.dfg.get_numeric_constant(**id).is_none()) - .copied(); - - let mut connected_sets_indices: HashSet = HashSet::new(); - - // Go through each parameter and each set and check if the set contains the parameter - // If it's the case, then that set doesn't present an issue - for parameter_or_return_value in variable_parameters_and_return_values { - for (set_index, final_set) in context.value_sets.iter().enumerate() { - if final_set.contains(¶meter_or_return_value) { - connected_sets_indices.insert(set_index); - } - } - } + context.compute_sets_of_connected_value_ids(function, all_functions); let all_brillig_generated_values: HashSet = context.brillig_return_to_argument.keys().copied().collect(); - // Go through each disconnected set + let connected_sets_indices = + context.find_sets_connected_to_function_inputs_or_outputs(function); + + // Go through each disconnected set, find brillig calls that caused it and form warnings for set_index in HashSet::from_iter(0..(context.value_sets.len())).difference(&connected_sets_indices) { let current_set = &context.value_sets[*set_index]; + warnings.append(&mut context.find_disconnecting_brillig_calls_with_results_in_set( + current_set, + &all_brillig_generated_values, + function, + )) + } + warnings +} +#[derive(Default)] +struct Context { + visited_blocks: HashSet, + block_queue: Vec, + value_sets: Vec>, + brillig_return_to_argument: HashMap>, + brillig_return_to_instruction_id: HashMap, +} + +impl Context { + /// Compute sets of variable ValueIds that are connected with constraints + /// + /// Additionally, store information about brillig calls in the context + fn compute_sets_of_connected_value_ids( + &mut self, + function: &Function, + all_functions: &BTreeMap, + ) { + // Go through each block in the function and create a list of sets of ValueIds connected by instructions + self.block_queue.push(function.entry_block()); + while let Some(block) = self.block_queue.pop() { + if self.visited_blocks.contains(&block) { + continue; + } + self.visited_blocks.insert(block); + self.connect_value_ids_in_block(function, block, all_functions); + } + + // Merge ValueIds into sets, where each original small set of ValueIds is merged with another set if they intersect + self.merge_sets(); + } + + /// Find sets that contain input or output value of the function + /// + /// Goes through each set of connected ValueIds and see if function arguments or return values are in the set + fn find_sets_connected_to_function_inputs_or_outputs( + &mut self, + function: &Function, + ) -> HashSet { + let variable_parameters_and_return_values = function + .parameters() + .iter() + .chain(function.returns()) + .filter(|id| function.dfg.get_numeric_constant(**id).is_none()) + .copied(); + + let mut connected_sets_indices: HashSet = HashSet::new(); + + // Go through each parameter and each set and check if the set contains the parameter + // If it's the case, then that set doesn't present an issue + for parameter_or_return_value in variable_parameters_and_return_values { + for (set_index, final_set) in self.value_sets.iter().enumerate() { + if final_set.contains(¶meter_or_return_value) { + connected_sets_indices.insert(set_index); + } + } + } + connected_sets_indices + } + /// Find which brillig calls separate this set from others and return bug warnings about them + fn find_disconnecting_brillig_calls_with_results_in_set( + &self, + current_set: &HashSet, + all_brillig_generated_values: &HashSet, + function: &Function, + ) -> Vec { + let mut warnings = Vec::new(); // Find brillig-generated values in the set let intersection = all_brillig_generated_values.intersection(current_set).copied(); @@ -88,7 +136,7 @@ fn check_for_underconstrained_values_within_function( for brillig_output_in_set in intersection { // Get the inputs that correspond to the output let inputs: HashSet = HashSet::from_iter( - context.brillig_return_to_argument[&brillig_output_in_set].iter().copied(), + self.brillig_return_to_argument[&brillig_output_in_set].iter().copied(), ); // Check if any of them are not in the set @@ -98,25 +146,16 @@ fn check_for_underconstrained_values_within_function( if unused_inputs { warnings.push(SsaReport::Bug(InternalBug::IndependentSubgraph { call_stack: function.dfg.get_call_stack( - context.brillig_return_to_instruction_id[&brillig_output_in_set], + self.brillig_return_to_instruction_id[&brillig_output_in_set], ), })); } } + warnings } - warnings -} -#[derive(Default)] -struct Context { - visited_blocks: HashSet, - block_queue: Vec, - value_sets: Vec>, - brillig_return_to_argument: HashMap>, - brillig_return_to_instruction_id: HashMap, -} - -impl Context { /// Go through each instruction in the block and add a set of ValueIds connected through that instruction + /// + /// Additionally, this function adds mappings of brillig return values to call arguments and instruction ids from calls to brillig functions in the block fn connect_value_ids_in_block( &mut self, function: &Function, From 21e73f50d779436cd49a7b54622c148a94751f69 Mon Sep 17 00:00:00 2001 From: Rumata888 Date: Fri, 5 Jul 2024 16:25:43 +0000 Subject: [PATCH 6/7] small fixes --- .../src/ssa/opt/check_for_underconstrained_values.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs index 9345f07a980..c76c5b60899 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs @@ -135,9 +135,8 @@ impl Context { // Go through all brillig outputs in the set for brillig_output_in_set in intersection { // Get the inputs that correspond to the output - let inputs: HashSet = HashSet::from_iter( - self.brillig_return_to_argument[&brillig_output_in_set].iter().copied(), - ); + let inputs: HashSet = + self.brillig_return_to_argument[&brillig_output_in_set].iter().copied().collect(); // Check if any of them are not in the set let unused_inputs = inputs.difference(current_set).next().is_some(); From d873e7b624f8fa36c8ca45bc6b123ce8201c845d Mon Sep 17 00:00:00 2001 From: Rumata888 Date: Fri, 5 Jul 2024 16:33:49 +0000 Subject: [PATCH 7/7] clippy --- .../src/ssa/opt/check_for_underconstrained_values.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs index c76c5b60899..6dac99d7a09 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/check_for_underconstrained_values.rs @@ -57,7 +57,7 @@ fn check_for_underconstrained_values_within_function( current_set, &all_brillig_generated_values, function, - )) + )); } warnings }