Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use super::{
instruction::{ConstrainError, InstructionId, Intrinsic},
types::NumericType,
},
opt::pure::FunctionPurities,
ssa_gen::Ssa,
};

Expand Down Expand Up @@ -57,7 +56,6 @@ pub struct FunctionBuilder {
allow_malformed_simplify: bool,

globals: Arc<GlobalsGraph>,
purities: Arc<FunctionPurities>,
}

impl FunctionBuilder {
Expand All @@ -76,7 +74,6 @@ impl FunctionBuilder {
simplify: true,
allow_malformed_simplify: false,
globals: Default::default(),
purities: Default::default(),
}
}

Expand All @@ -90,13 +87,11 @@ impl FunctionBuilder {
}

/// Create a function builder with a new function created with the same
/// name, globals, and function purities taken from an existing function.
/// name and globals taken from an existing function.
pub fn from_existing(function: &Function, function_id: FunctionId) -> Self {
let mut this = Self::new(function.name().to_owned(), function_id);
this.set_globals(function.dfg.globals.clone());
this.purities = function.dfg.function_purities.clone();
this.current_function.set_runtime(function.runtime());
this.current_function.dfg.set_function_purities(this.purities.clone());
this.set_allow_malformed_simplify(function.dfg.allow_malformed_simplify);
this.current_function.dfg.allow_constant_return = function.dfg.allow_constant_return;
this
Expand Down Expand Up @@ -127,11 +122,6 @@ impl FunctionBuilder {
self.current_function.set_globals(self.globals.clone());
}

pub(crate) fn set_purities(&mut self, purities: Arc<FunctionPurities>) {
self.purities = purities.clone();
self.current_function.dfg.set_function_purities(purities);
}

/// Finish the current function and create a new function.
///
/// A `FunctionBuilder` can always only work on one function at a time, so care
Expand All @@ -154,7 +144,6 @@ impl FunctionBuilder {
self.current_function.dfg.call_stack_data.get_or_insert_locations(&call_stack);
self.finished_functions.push(old_function);

self.current_function.dfg.set_function_purities(self.purities.clone());
self.current_function.dfg.allow_malformed_simplify = self.allow_malformed_simplify;
self.apply_globals();
}
Expand Down
23 changes: 1 addition & 22 deletions compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,8 @@
use crate::{
brillig::assert_u32,
ssa::{
RuntimeError,
function_builder::data_bus::DataBus,
ir::function::Function,
RuntimeError, function_builder::data_bus::DataBus, ir::function::Function,
ir::instruction::ArrayOffset,
opt::pure::{FunctionPurities, Purity},
},
};

Expand Down Expand Up @@ -118,9 +115,6 @@

pub(crate) globals: Arc<GlobalsGraph>,

#[serde(skip)]
pub(crate) function_purities: Arc<FunctionPurities>,

/// Indicate whether the Brillig array index offset optimizations have been performed.
pub(crate) brillig_arrays_offset: bool,

Expand Down Expand Up @@ -662,12 +656,12 @@
/// True if `value` is boolean: a 0/1 constant, a `u1`-typed value, or a chain of casts
/// bottoming out at one of those.
///
/// This trusts the static type: a `u1`-typed value is assumed to hold 0 or 1, the canonicality

Check warning on line 659 in compiler/noirc_evaluator/src/ssa/ir/dfg.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (canonicality)
/// invariant the compiler maintains for every value it creates (in ACIR a `u1` unchecked
/// add/sub result can transiently violate it, but such values only ever flow into truncations
/// and casts). Use this for algebraic rewrites that are valid for canonical values, such as
/// `b*b = b`. Do NOT use it to justify deleting a range check: for that,
/// [`Self::get_value_max_num_bits`] gives a bound that does not assume canonicality of

Check warning on line 664 in compiler/noirc_evaluator/src/ssa/ir/dfg.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (canonicality)
/// unchecked arithmetic results.
pub(crate) fn is_boolean_value(&self, value: ValueId) -> bool {
if let Some(constant) = self.get_numeric_constant(value) {
Expand Down Expand Up @@ -1006,21 +1000,6 @@
}
}

pub(crate) fn set_function_purities(&mut self, purities: Arc<FunctionPurities>) {
self.function_purities = purities;
}

/// Returns the purity of `function` as observed from this function (the caller).
///
/// This is the callee's own purity, except that a pure Brillig function called from an ACIR
/// function is observed as [Purity::PureWithPredicate]: the call lowers to a predicated
/// `Opcode::BrilligCall` whose outputs are left unconstrained when the predicate is disabled,
/// so the result is predicate-dependent from an ACIR caller's perspective. From a Brillig
/// caller (whose calls are not predicated) the function's true purity is observed.
pub(crate) fn purity_of(&self, function: FunctionId) -> Option<Purity> {
self.function_purities.purity_of(function, self.runtime())
}

/// Determine the appropriate [`ArrayOffset`] to use for indexing an array or vector.
pub(crate) fn array_offset(&self, array: ValueId, index: ValueId) -> ArrayOffset {
if !self.runtime.is_brillig()
Expand Down
1 change: 0 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ impl Function {
let mut new_function = Function::new(another.name.clone(), id);
new_function.set_runtime(another.runtime());
new_function.set_globals(another.dfg.globals.clone());
new_function.dfg.set_function_purities(another.dfg.function_purities.clone());
new_function.dfg.brillig_arrays_offset = another.dfg.brillig_arrays_offset;
new_function.dfg.allow_constant_return = another.dfg.allow_constant_return;
new_function
Expand Down
15 changes: 12 additions & 3 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ use acvm::{
use iter_extended::vecmap;
use noirc_frontend::hir_def::types::Type as HirType;

use crate::ssa::{ir::integer::IntegerConstant, opt::pure::Purity};
use crate::ssa::{
ir::integer::IntegerConstant,
opt::pure::{FunctionPurities, Purity},
};

use super::{
basic_block::BasicBlockId,
Expand Down Expand Up @@ -592,7 +595,11 @@ impl Instruction {
}

/// Indicates if the instruction has a side effect, ie. it can fail, or it interacts with memory.
pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool {
pub(crate) fn has_side_effects(
&self,
dfg: &DataFlowGraph,
purities: &FunctionPurities,
) -> bool {
use Instruction::*;

match self {
Expand All @@ -608,7 +615,9 @@ impl Instruction {
Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(),
// Functions known to be pure have no side effects.
// `PureWithPredicates` functions may still have side effects.
Value::Function(function) => dfg.purity_of(function) != Some(Purity::Pure),
Value::Function(function) => {
purities.purity_of(function, dfg.runtime()) != Some(Purity::Pure)
}
_ => true, // Be conservative and assume other functions can have side effects.
},

Expand Down
12 changes: 9 additions & 3 deletions compiler/noirc_evaluator/src/ssa/ir/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::ssa::{
instruction::ArrayOffset,
types::{NumericType, Type},
},
opt::pure::FunctionPurities,
};

use super::{
Expand Down Expand Up @@ -69,7 +70,7 @@ impl Display for Printer<'_> {
}

for function in self.ssa.functions.values() {
display_function(function, self.fm, f)?;
display_function(function, self.fm, Some(&self.ssa.function_purities), f)?;
writeln!(f)?;
}
Ok(())
Expand All @@ -78,17 +79,22 @@ impl Display for Printer<'_> {

impl Display for Function {
fn fmt(&self, f: &mut Formatter<'_>) -> Result {
display_function(self, None, f)
display_function(self, None, None, f)
}
}

/// Helper function for Function's Display impl to pretty-print the function with the given formatter.
///
/// Purities live on the [Ssa] rather than on each function, so a
/// standalone [Function] display (which has no `purities`) omits the purity keyword.
fn display_function(
function: &Function,
files: Option<&fm::FileManager>,
purities: Option<&FunctionPurities>,
f: &mut Formatter,
) -> Result {
if let Some(purity) = function.dfg.purity_of(function.id()) {
let purity = purities.and_then(|purities| purities.intrinsic_purity_of(function.id()));
if let Some(purity) = purity {
writeln!(f, "{} {purity} fn {} {} {{", function.runtime(), function.name(), function.id())?;
} else {
writeln!(f, "{} fn {} {} {{", function.runtime(), function.name(), function.id())?;
Expand Down
12 changes: 9 additions & 3 deletions compiler/noirc_evaluator/src/ssa/ir/target_cost.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
//! If ACIR cost estimation is needed in the future, it can be added here alongside the
//! Brillig estimates.

use crate::ssa::opt::pure::FunctionPurities;

use super::{
dfg::DataFlowGraph,
function::Function,
Expand All @@ -33,11 +35,15 @@
///
/// `DecrementRc` is **not** safe to hoist: running it on a path that did not
/// before *lowers* a reference count, which can enable an unsafe in-place
/// mutation. It is reported as non-flattenable.

Check warning on line 38 in compiler/noirc_evaluator/src/ssa/ir/target_cost.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (flattenable)
///
/// Div/Mod and Shl/Shr are blocked unconditionally — even when `has_side_effects`
/// would allow them (e.g. known non-zero divisor), they are rarely worth flattening.
pub(crate) fn can_flatten_in_conditional(&self, dfg: &DataFlowGraph) -> bool {
pub(crate) fn can_flatten_in_conditional(
&self,
dfg: &DataFlowGraph,
purities: &FunctionPurities,
) -> bool {
match self {
Instruction::EnableSideEffectsIf { .. } => {
if dfg.runtime().is_brillig() {
Expand All @@ -58,10 +64,10 @@

Instruction::Binary(binary) => match binary.operator {
BinaryOp::Div | BinaryOp::Mod | BinaryOp::Shl | BinaryOp::Shr => false,
_ => !self.has_side_effects(dfg),
_ => !self.has_side_effects(dfg, purities),
},

_ => !self.has_side_effects(dfg),
_ => !self.has_side_effects(dfg, purities),
}
}
}
Expand Down
29 changes: 19 additions & 10 deletions compiler/noirc_evaluator/src/ssa/opt/basic_conditional.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::ssa::{
types::Type,
value::ValueId,
},
opt::flatten_cfg::WorkList,
opt::{flatten_cfg::WorkList, pure::FunctionPurities},
};

use super::flatten_cfg::Context;
Expand All @@ -47,7 +47,7 @@ impl Ssa {
self.functions.values().filter(|f| f.is_no_predicates()).map(|f| f.id()).collect();

for function in self.functions.values_mut() {
flatten_function(function, &no_predicates);
flatten_function(function, &no_predicates, &self.function_purities);
}
self
}
Expand Down Expand Up @@ -76,6 +76,7 @@ fn is_conditional(
block: BasicBlockId,
cfg: &ControlFlowGraph,
function: &Function,
purities: &FunctionPurities,
) -> Option<BasicConditional> {
// A conditional must end with a JmpIf
let Some(TerminatorInstruction::JmpIf {
Expand Down Expand Up @@ -113,8 +114,8 @@ fn is_conditional(
// \ /
// next_then
// We check that the cost of the flattened code is lower than the cost of the branches
let cost_left = block_flatten_cost(*then_destination, &function.dfg)?;
let cost_right = block_flatten_cost(*else_destination, &function.dfg)?;
let cost_left = block_flatten_cost(*then_destination, &function.dfg, purities)?;
let cost_right = block_flatten_cost(*else_destination, &function.dfg, purities)?;
// Compute the actual branching overhead for this conditional:
// Flattening eliminates: JmpIf + then's Jmp + else's Jmp
// Flattening adds merge (IfElse) ops only for exit params where branches differ.
Expand Down Expand Up @@ -151,7 +152,7 @@ fn is_conditional(
if !then_arguments.is_empty() || !else_arguments.is_empty() {
return None;
}
let cost = block_flatten_cost(*then_destination, &function.dfg)?;
let cost = block_flatten_cost(*then_destination, &function.dfg, purities)?;
// Flattening eliminates: JmpIf + then's Jmp; adds IfElse per exit param
let then_term_cost = function.dfg[*then_destination].unwrap_terminator().cost();
let merge_cost = function.dfg.block_parameters(*else_destination).len() * 3;
Expand Down Expand Up @@ -180,7 +181,7 @@ fn is_conditional(
if !then_arguments.is_empty() || !else_arguments.is_empty() {
return None;
}
let cost = block_flatten_cost(*else_destination, &function.dfg)?;
let cost = block_flatten_cost(*else_destination, &function.dfg, purities)?;
// Flattening eliminates: JmpIf + else's Jmp; adds IfElse per exit param
let else_term_cost = function.dfg[*else_destination].unwrap_terminator().cost();
let merge_cost = function.dfg.block_parameters(*then_destination).len() * 3;
Expand Down Expand Up @@ -254,7 +255,11 @@ fn differing_merge_cost(
/// reaching them as an ICE.) Hoisting an `inc_rc` only ever raises a reference
/// count, so the later `array_set` copies rather than mutating in place — sound,
/// and guarded by the `rc_invariant::array_set` validator.
fn block_flatten_cost(block: BasicBlockId, dfg: &DataFlowGraph) -> Option<u32> {
fn block_flatten_cost(
block: BasicBlockId,
dfg: &DataFlowGraph,
purities: &FunctionPurities,
) -> Option<u32> {
let mut cost: u32 = 0;
for instruction_id in dfg[block].instructions() {
let instruction = &dfg[*instruction_id];
Expand All @@ -267,7 +272,7 @@ fn block_flatten_cost(block: BasicBlockId, dfg: &DataFlowGraph) -> Option<u32> {
continue;
}

if !instruction.can_flatten_in_conditional(dfg) {
if !instruction.can_flatten_in_conditional(dfg, purities) {
return None;
}

Expand All @@ -277,7 +282,11 @@ fn block_flatten_cost(block: BasicBlockId, dfg: &DataFlowGraph) -> Option<u32> {
}

/// Identifies all simple conditionals in the function and flattens them
fn flatten_function(function: &mut Function, no_predicates: &HashSet<FunctionId>) {
fn flatten_function(
function: &mut Function,
no_predicates: &HashSet<FunctionId>,
purities: &FunctionPurities,
) {
// This pass is dedicated to brillig functions
if !function.runtime().is_brillig() {
return;
Expand All @@ -297,7 +306,7 @@ fn flatten_function(function: &mut Function, no_predicates: &HashSet<FunctionId>
processed.insert(block);

// Identify the simple conditionals
if let Some(conditional) = is_conditional(block, &cfg, function) {
if let Some(conditional) = is_conditional(block, &cfg, function, purities) {
// no need to check the branches, process the join block directly
stack.push(conditional.block_exit);
conditionals.push(conditional);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@

// A shifted index that doesn't fit the u32 addressing type, or that lands outside the
// addressable Brillig memory range, can never be a valid address. Leave the access
// unshifted and let the normal runtime out-of-bounds path handle it instead of

Check warning on line 132 in compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (unshifted)
// producing a constant the backend would reject.
let fits_in_memory =
shifted_index.try_to_u32().is_some_and(|index| index as usize <= MAX_MEMORY_SIZE);
Expand Down Expand Up @@ -301,7 +301,7 @@
let func = ssa.main();
let b0 = &func.dfg[func.entry_block()];
let instruction = &func.dfg[b0.instructions()[0]];
instruction.has_side_effects(&func.dfg)
instruction.has_side_effects(&func.dfg, &ssa.function_purities)
}

let ssa = Ssa::from_str(src).unwrap();
Expand All @@ -323,7 +323,7 @@
fn do_not_offset_vector_when_shifted_index_overflows_addressing_bits() {
// The vector offset is 3, so shifting u32::MAX would produce 4294967298,
// which does not fit in the 32-bit Brillig addressing type. The pass must
// leave the stored index unshifted so it is handled by the normal runtime

Check warning on line 326 in compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (unshifted)
// out-of-bounds path instead of producing a constant Brillig codegen rejects.
// The cosmetic " minus 3" is added by the printer once arrays are offset, but
// the printed value stays at 4294967295 rather than the overflowing 4294967298.
Expand Down Expand Up @@ -352,8 +352,8 @@
// 2147483647 (i32::MAX) fits the 32-bit addressing type, but shifting it by
// the vector offset lands above the maximum addressable memory slot
// (MAX_MEMORY_SIZE == i32::MAX), so it can never be a valid address. The pass
// must leave it unshifted for the runtime out-of-bounds path. The printed value

Check warning on line 355 in compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (unshifted)
// stays at 2147483647 rather than the unaddressable 2147483650.

Check warning on line 356 in compiler/noirc_evaluator/src/ssa/opt/brillig_array_get_and_set.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (unaddressable)
let src = "
brillig(inline) fn main f0 {
b0(v0: [Field]):
Expand Down
Loading
Loading