From a72daa4764310078ab0c70720f16c85b2c0375bb Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 11 Aug 2023 03:29:52 -0500 Subject: [PATCH] feat: Add `assert_constant` (#2242) * Add assert_constant * Remove println & dbg * Add test --- .../assert_constant_fail/Nargo.toml | 7 ++ .../assert_constant_fail/src/main.nr | 10 +++ crates/noirc_evaluator/src/errors.rs | 5 +- crates/noirc_evaluator/src/ssa.rs | 1 + .../noirc_evaluator/src/ssa/ir/basic_block.rs | 5 ++ crates/noirc_evaluator/src/ssa/ir/dfg.rs | 11 ++- .../noirc_evaluator/src/ssa/ir/instruction.rs | 3 + .../src/ssa/ir/instruction/call.rs | 7 ++ .../src/ssa/opt/assert_constant.rs | 83 +++++++++++++++++++ .../src/ssa/opt/constant_folding.rs | 2 +- crates/noirc_evaluator/src/ssa/opt/mod.rs | 1 + noir_stdlib/src/lib.nr | 5 ++ 12 files changed, 134 insertions(+), 6 deletions(-) create mode 100644 crates/nargo_cli/tests/compile_failure/assert_constant_fail/Nargo.toml create mode 100644 crates/nargo_cli/tests/compile_failure/assert_constant_fail/src/main.nr create mode 100644 crates/noirc_evaluator/src/ssa/opt/assert_constant.rs diff --git a/crates/nargo_cli/tests/compile_failure/assert_constant_fail/Nargo.toml b/crates/nargo_cli/tests/compile_failure/assert_constant_fail/Nargo.toml new file mode 100644 index 00000000000..a8c9b4ad344 --- /dev/null +++ b/crates/nargo_cli/tests/compile_failure/assert_constant_fail/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "assert_constant_fail" +type = "bin" +authors = [""] +compiler_version = "0.9.0" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/compile_failure/assert_constant_fail/src/main.nr b/crates/nargo_cli/tests/compile_failure/assert_constant_fail/src/main.nr new file mode 100644 index 00000000000..cf682607083 --- /dev/null +++ b/crates/nargo_cli/tests/compile_failure/assert_constant_fail/src/main.nr @@ -0,0 +1,10 @@ +use dep::std::assert_constant; + +fn main(x: Field) { + foo(5, x); +} + +fn foo(constant: Field, non_constant: Field) { + assert_constant(constant); + assert_constant(non_constant); +} diff --git a/crates/noirc_evaluator/src/errors.rs b/crates/noirc_evaluator/src/errors.rs index 981f0c9f8a7..aebadfcd0ae 100644 --- a/crates/noirc_evaluator/src/errors.rs +++ b/crates/noirc_evaluator/src/errors.rs @@ -32,6 +32,8 @@ pub enum RuntimeError { UnsupportedIntegerSize { num_bits: u32, max_num_bits: u32, location: Option }, #[error("Could not determine loop bound at compile-time")] UnknownLoopBound { location: Option }, + #[error("Argument is not constant")] + AssertConstantFailed { location: Option }, } #[derive(Debug, PartialEq, Eq, Clone, Error)] @@ -69,7 +71,8 @@ impl RuntimeError { | RuntimeError::InvalidRangeConstraint { location, .. } | RuntimeError::TypeConversion { location, .. } | RuntimeError::UnInitialized { location, .. } - | RuntimeError::UnknownLoopBound { location, .. } + | RuntimeError::UnknownLoopBound { location } + | RuntimeError::AssertConstantFailed { location } | RuntimeError::UnsupportedIntegerSize { location, .. } => *location, } } diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index 5f29027669a..10099037dfe 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -49,6 +49,7 @@ pub(crate) fn optimize_into_acir( ssa = ssa .inline_functions() .print(print_ssa_passes, "After Inlining:") + .evaluate_assert_constant()? .unroll_loops()? .print(print_ssa_passes, "After Unrolling:") .simplify_cfg() diff --git a/crates/noirc_evaluator/src/ssa/ir/basic_block.rs b/crates/noirc_evaluator/src/ssa/ir/basic_block.rs index 4886e3f5da8..998591f7210 100644 --- a/crates/noirc_evaluator/src/ssa/ir/basic_block.rs +++ b/crates/noirc_evaluator/src/ssa/ir/basic_block.rs @@ -73,6 +73,11 @@ impl BasicBlock { &mut self.instructions } + /// Take the instructions in this block, replacing it with an empty Vec + pub(crate) fn take_instructions(&mut self) -> Vec { + std::mem::take(&mut self.instructions) + } + /// Sets the terminator instruction of this block. /// /// A properly-constructed block will always terminate with a TerminatorInstruction - diff --git a/crates/noirc_evaluator/src/ssa/ir/dfg.rs b/crates/noirc_evaluator/src/ssa/ir/dfg.rs index f8b5146b2ed..28879e99701 100644 --- a/crates/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa/ir/dfg.rs @@ -399,7 +399,7 @@ impl DataFlowGraph { /// source block. pub(crate) fn inline_block(&mut self, source: BasicBlockId, destination: BasicBlockId) { let source = &mut self.blocks[source]; - let mut instructions = std::mem::take(source.instructions_mut()); + let mut instructions = source.take_instructions(); let terminator = source.take_terminator(); let destination = &mut self.blocks[destination]; @@ -417,6 +417,11 @@ impl DataFlowGraph { _ => None, } } + + /// True if the given ValueId refers to a constant value + pub(crate) fn is_constant(&self, argument: ValueId) -> bool { + !matches!(&self[self.resolve(argument)], Value::Instruction { .. } | Value::Param { .. }) + } } impl std::ops::Index for DataFlowGraph { @@ -483,9 +488,7 @@ impl<'dfg> InsertInstructionResult<'dfg> { InsertInstructionResult::Results(results) => Cow::Borrowed(results), InsertInstructionResult::SimplifiedTo(result) => Cow::Owned(vec![result]), InsertInstructionResult::SimplifiedToMultiple(results) => Cow::Owned(results), - InsertInstructionResult::InstructionRemoved => { - panic!("InsertInstructionResult::results called on a removed instruction") - } + InsertInstructionResult::InstructionRemoved => Cow::Owned(vec![]), } } diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index 3b8ec03c125..31521def27f 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -34,6 +34,7 @@ pub(crate) type InstructionId = Id; pub(crate) enum Intrinsic { Sort, ArrayLen, + AssertConstant, SlicePushBack, SlicePushFront, SlicePopBack, @@ -52,6 +53,7 @@ impl std::fmt::Display for Intrinsic { Intrinsic::Println => write!(f, "println"), Intrinsic::Sort => write!(f, "arraysort"), Intrinsic::ArrayLen => write!(f, "array_len"), + Intrinsic::AssertConstant => write!(f, "assert_constant"), Intrinsic::SlicePushBack => write!(f, "slice_push_back"), Intrinsic::SlicePushFront => write!(f, "slice_push_front"), Intrinsic::SlicePopBack => write!(f, "slice_pop_back"), @@ -75,6 +77,7 @@ impl Intrinsic { "println" => Some(Intrinsic::Println), "arraysort" => Some(Intrinsic::Sort), "array_len" => Some(Intrinsic::ArrayLen), + "assert_constant" => Some(Intrinsic::AssertConstant), "slice_push_back" => Some(Intrinsic::SlicePushBack), "slice_push_front" => Some(Intrinsic::SlicePushFront), "slice_pop_back" => Some(Intrinsic::SlicePopBack), diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs index a28f2d88bcb..648bffd093a 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -165,6 +165,13 @@ pub(super) fn simplify_call( SimplifyResult::None } } + Intrinsic::AssertConstant => { + if arguments.iter().all(|argument| dfg.is_constant(*argument)) { + SimplifyResult::Remove + } else { + SimplifyResult::None + } + } Intrinsic::BlackBox(bb_func) => simplify_black_box_func(bb_func, arguments, dfg), Intrinsic::Sort => simplify_sort(dfg, arguments), Intrinsic::Println => SimplifyResult::None, diff --git a/crates/noirc_evaluator/src/ssa/opt/assert_constant.rs b/crates/noirc_evaluator/src/ssa/opt/assert_constant.rs new file mode 100644 index 00000000000..81cee2ffde4 --- /dev/null +++ b/crates/noirc_evaluator/src/ssa/opt/assert_constant.rs @@ -0,0 +1,83 @@ +use crate::{ + errors::RuntimeError, + ssa::{ + ir::{ + function::Function, + instruction::{Instruction, InstructionId, Intrinsic}, + value::ValueId, + }, + ssa_gen::Ssa, + }, +}; + +impl Ssa { + /// A simple SSA pass to go through each instruction and evaluate each call + /// to `assert_constant`, issuing an error if any arguments to the function are + /// not constants. + /// + /// Note that this pass must be placed directly before loop unrolling to be + /// useful. Any optimization passes between this and loop unrolling will cause + /// the constants that this pass sees to be potentially different than the constants + /// seen by loop unrolling. Furthermore, this pass cannot be a part of loop unrolling + /// since we must go through every instruction to find all references to `assert_constant` + /// while loop unrolling only touches blocks with loops in them. + pub(crate) fn evaluate_assert_constant(mut self) -> Result { + for function in self.functions.values_mut() { + for block in function.reachable_blocks() { + // Unfortunately we can't just use instructions.retain(...) here since + // check_instruction can also return an error + let instructions = function.dfg[block].take_instructions(); + let mut filtered_instructions = Vec::with_capacity(instructions.len()); + + for instruction in instructions { + if check_instruction(function, instruction)? { + filtered_instructions.push(instruction); + } + } + + *function.dfg[block].instructions_mut() = filtered_instructions; + } + } + Ok(self) + } +} + +/// During the loop unrolling pass we also evaluate calls to `assert_constant`. +/// This is done in this pass because loop unrolling is the only pass that will error +/// if a value (the loop bounds) are not known constants. +/// +/// This returns Ok(true) if the given instruction should be kept in the block and +/// Ok(false) if it should be removed. +fn check_instruction( + function: &mut Function, + instruction: InstructionId, +) -> Result { + let assert_constant_id = function.dfg.import_intrinsic(Intrinsic::AssertConstant); + match &function.dfg[instruction] { + Instruction::Call { func, arguments } => { + if *func == assert_constant_id { + evaluate_assert_constant(function, instruction, arguments) + } else { + Ok(true) + } + } + _ => Ok(true), + } +} + +/// Evaluate a call to `assert_constant`, returning an error if any of the elements are not +/// constants. If all of the elements are constants, Ok(false) is returned. This signifies a +/// success but also that the instruction need not be reinserted into the block being unrolled +/// since it has already been evaluated. +fn evaluate_assert_constant( + function: &Function, + instruction: InstructionId, + arguments: &[ValueId], +) -> Result { + if arguments.iter().all(|arg| function.dfg.is_constant(*arg)) { + Ok(false) + } else { + let location = function.dfg.get_location(&instruction); + Err(RuntimeError::AssertConstantFailed { location }) + } +} diff --git a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs index ea46ddf1d4f..c4f3184f794 100644 --- a/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/crates/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -50,7 +50,7 @@ struct Context { impl Context { fn fold_constants_in_block(&mut self, function: &mut Function, block: BasicBlockId) { - let instructions = std::mem::take(function.dfg[block].instructions_mut()); + let instructions = function.dfg[block].take_instructions(); for instruction in instructions { self.push_instruction(function, block, instruction); diff --git a/crates/noirc_evaluator/src/ssa/opt/mod.rs b/crates/noirc_evaluator/src/ssa/opt/mod.rs index 23b4c726a6b..9b9350118ad 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mod.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mod.rs @@ -4,6 +4,7 @@ //! simpler form until the IR only has a single function remaining with 1 block within it. //! Generally, these passes are also expected to minimize the final amount of instructions. mod array_use; +mod assert_constant; mod constant_folding; mod defunctionalize; mod die; diff --git a/noir_stdlib/src/lib.nr b/noir_stdlib/src/lib.nr index 9c0dcc6b269..d552ca44a29 100644 --- a/noir_stdlib/src/lib.nr +++ b/noir_stdlib/src/lib.nr @@ -27,3 +27,8 @@ unconstrained fn println(input: T) { #[foreign(recursive_aggregation)] fn verify_proof(_verification_key : [Field], _proof : [Field], _public_inputs : [Field], _key_hash : Field, _input_aggregation_object : [Field; N]) -> [Field; N] {} + +// Asserts that the given value is known at compile-time. +// Useful for debugging for-loop bounds. +#[builtin(assert_constant)] +fn assert_constant(_x: T) {}