From 342085cb27979574274a3a0b55365736e880ae67 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 22 Jan 2024 17:36:24 +0000 Subject: [PATCH 1/4] fix: zero out input to `to_radix` calls if inactive --- .../src/ssa/opt/flatten_cfg.rs | 27 +++++++++++++++++-- .../execution_success/to_le_bytes/Prover.toml | 1 + .../execution_success/to_le_bytes/src/main.nr | 9 ++++++- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 6bdf2ab1c0a..1ccd7b71c17 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -144,9 +144,9 @@ use crate::ssa::{ dfg::{CallStack, InsertInstructionResult}, function::Function, function_inserter::FunctionInserter, - instruction::{BinaryOp, Instruction, InstructionId, TerminatorInstruction}, + instruction::{BinaryOp, Instruction, InstructionId, Intrinsic, TerminatorInstruction}, types::Type, - value::ValueId, + value::{Value, ValueId}, }, ssa_gen::Ssa, }; @@ -683,6 +683,29 @@ impl<'f> Context<'f> { ); Instruction::RangeCheck { value, max_bit_size, assert_message } } + Instruction::Call { func, mut arguments } => match self.inserter.function.dfg[func] + { + Value::Intrinsic(Intrinsic::ToBits(_) | Intrinsic::ToRadix(_)) => { + let field = arguments[0]; + arguments.remove(0); + + let argument_type = self.inserter.function.dfg.type_of_value(field); + let casted_condition = self.insert_instruction( + Instruction::Cast(condition, argument_type), + call_stack.clone(), + ); + let field = self.insert_instruction( + Instruction::binary(BinaryOp::Mul, field, casted_condition), + call_stack.clone(), + ); + + arguments.insert(0, field); + + Instruction::Call { func, arguments } + } + + _ => Instruction::Call { func, arguments }, + }, other => other, } } else { diff --git a/test_programs/execution_success/to_le_bytes/Prover.toml b/test_programs/execution_success/to_le_bytes/Prover.toml index 07fe857ac7c..bf58776d557 100644 --- a/test_programs/execution_success/to_le_bytes/Prover.toml +++ b/test_programs/execution_success/to_le_bytes/Prover.toml @@ -1 +1,2 @@ x = "2040124" +cond = false \ No newline at end of file diff --git a/test_programs/execution_success/to_le_bytes/src/main.nr b/test_programs/execution_success/to_le_bytes/src/main.nr index 05eefc0f143..a0b48efe528 100644 --- a/test_programs/execution_success/to_le_bytes/src/main.nr +++ b/test_programs/execution_success/to_le_bytes/src/main.nr @@ -1,4 +1,4 @@ -fn main(x: Field) -> pub [u8; 31] { +fn main(x: Field, cond: bool) -> pub [u8; 31] { // The result of this byte array will be little-endian let byte_array = x.to_le_bytes(31); assert(byte_array.len() == 31); @@ -7,5 +7,12 @@ fn main(x: Field) -> pub [u8; 31] { for i in 0..31 { bytes[i] = byte_array[i]; } + + if cond { + // We've set x = "2040124" so we shouldn't be able to represent this as a single byte. + let bad_byte_array = x.to_le_bytes(1); + assert_eq(bad_byte_array.len(), 1); + } + bytes } From 04cf809a1d05b9010df9a1cd4250cda639f9696f Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 25 Jan 2024 11:28:15 +0000 Subject: [PATCH 2/4] feat: mark `to_radix` as side-effectful --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 457fe41de93..e123623cae9 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -82,6 +82,9 @@ impl Intrinsic { match self { Intrinsic::AssertConstant | Intrinsic::ApplyRangeConstraint => true, + // These apply a constraint that the input must fit into a specified number of limbs. + Intrinsic::ToBits(_) | Intrinsic::ToRadix(_) => true, + Intrinsic::Sort | Intrinsic::ArrayLen | Intrinsic::SlicePushBack @@ -91,8 +94,6 @@ impl Intrinsic { | Intrinsic::SliceInsert | Intrinsic::SliceRemove | Intrinsic::StrAsBytes - | Intrinsic::ToBits(_) - | Intrinsic::ToRadix(_) | Intrinsic::FromField | Intrinsic::AsField => false, From 23f9c05de0752cd6d84ca4f3d0beda18a50aaf97 Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 25 Jan 2024 11:29:31 +0000 Subject: [PATCH 3/4] chore: remove `remove` and `insert` calls --- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 1ccd7b71c17..1059994b9be 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -687,9 +687,8 @@ impl<'f> Context<'f> { { Value::Intrinsic(Intrinsic::ToBits(_) | Intrinsic::ToRadix(_)) => { let field = arguments[0]; - arguments.remove(0); - let argument_type = self.inserter.function.dfg.type_of_value(field); + let casted_condition = self.insert_instruction( Instruction::Cast(condition, argument_type), call_stack.clone(), @@ -699,7 +698,7 @@ impl<'f> Context<'f> { call_stack.clone(), ); - arguments.insert(0, field); + arguments[0] = field; Instruction::Call { func, arguments } } From 822fd2e140eacbabd295d4e375ff6bbfb3b426ab Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 25 Jan 2024 11:44:43 +0000 Subject: [PATCH 4/4] chore: update DIE test to account for to_radix having side-effects --- test_programs/compile_success_empty/intrinsic_die/src/main.nr | 2 -- 1 file changed, 2 deletions(-) diff --git a/test_programs/compile_success_empty/intrinsic_die/src/main.nr b/test_programs/compile_success_empty/intrinsic_die/src/main.nr index 88f7a3634c1..8cac707dfea 100644 --- a/test_programs/compile_success_empty/intrinsic_die/src/main.nr +++ b/test_programs/compile_success_empty/intrinsic_die/src/main.nr @@ -1,8 +1,6 @@ use dep::std; // This test checks that we perform dead-instruction-elimination on intrinsic functions. fn main(x: Field) { - let bytes = x.to_be_bytes(32); - let hash = std::hash::pedersen_commitment([x]); let _p1 = std::scalar_mul::fixed_base_embedded_curve(x, 0); }