Skip to content

Commit

Permalink
fix: disable side-effects for no_predicates functions (#6027)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Brillig output is nullified if the call is with a predicate, however,
functions with no_predicates attribute expect valid values from their
brillig calls because when they constrain the values , it will not have
a predicate.

Resolves e-2-e test failing on PR
AztecProtocol/aztec-packages#4571

## Summary\*
Disable side-effects when calling a function with no_predicates



## Additional Context
The dedicated inlining pass for no_predicates functions can be removed
after this fix.


## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [X] I have tested the changes locally.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
guipublic authored Sep 13, 2024
1 parent e74b4ae commit fc74c55
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 8 deletions.
53 changes: 45 additions & 8 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ use crate::ssa::{
basic_block::BasicBlockId,
cfg::ControlFlowGraph,
dfg::{CallStack, InsertInstructionResult},
function::Function,
function::{Function, FunctionId},
function_inserter::FunctionInserter,
instruction::{BinaryOp, Instruction, InstructionId, Intrinsic, TerminatorInstruction},
types::Type,
Expand All @@ -164,8 +164,14 @@ impl Ssa {
/// For more information, see the module-level comment at the top of this file.
#[tracing::instrument(level = "trace", skip(self))]
pub(crate) fn flatten_cfg(mut self) -> Ssa {
// Retrieve the 'no_predicates' attribute of the functions in a map, to avoid problems with borrowing
let mut no_predicates = HashMap::default();
for function in self.functions.values() {
no_predicates.insert(function.id(), function.is_no_predicates());
}

for function in self.functions.values_mut() {
flatten_function_cfg(function);
flatten_function_cfg(function, &no_predicates);
}
self
}
Expand Down Expand Up @@ -244,7 +250,7 @@ struct ConditionalContext {
call_stack: CallStack,
}

fn flatten_function_cfg(function: &mut Function) {
fn flatten_function_cfg(function: &mut Function, no_predicates: &HashMap<FunctionId, bool>) {
// This pass may run forever on a brillig function.
// Analyze will check if the predecessors have been processed and push the block to the back of
// the queue. This loops forever if there are still any loops present in the program.
Expand All @@ -264,18 +270,18 @@ fn flatten_function_cfg(function: &mut Function) {
condition_stack: Vec::new(),
arguments_stack: Vec::new(),
};
context.flatten();
context.flatten(no_predicates);
}

impl<'f> Context<'f> {
fn flatten(&mut self) {
fn flatten(&mut self, no_predicates: &HashMap<FunctionId, bool>) {
// Flatten the CFG by inlining all instructions from the queued blocks
// until all blocks have been flattened.
// We follow the terminator of each block to determine which blocks to
// process next
let mut queue = vec![self.inserter.function.entry_block()];
while let Some(block) = queue.pop() {
self.inline_block(block);
self.inline_block(block, no_predicates);
let to_process = self.handle_terminator(block, &queue);
for incoming_block in to_process {
if !queue.contains(&incoming_block) {
Expand Down Expand Up @@ -307,8 +313,23 @@ impl<'f> Context<'f> {
})
}

/// Use the provided map to say if the instruction is a call to a no_predicates function
fn is_no_predicate(
&self,
no_predicates: &HashMap<FunctionId, bool>,
instruction: &InstructionId,
) -> bool {
let mut result = false;
if let Instruction::Call { func, .. } = self.inserter.function.dfg[*instruction] {
if let Value::Function(fid) = self.inserter.function.dfg[func] {
result = *no_predicates.get(&fid).unwrap_or(&false);
}
}
result
}

// Inline all instructions from the given block into the entry block, and track slice capacities
fn inline_block(&mut self, block: BasicBlockId) {
fn inline_block(&mut self, block: BasicBlockId, no_predicates: &HashMap<FunctionId, bool>) {
if self.inserter.function.entry_block() == block {
// we do not inline the entry block into itself
// for the outer block before we start inlining
Expand All @@ -322,7 +343,23 @@ impl<'f> Context<'f> {
// unnecessary, when removing it actually causes an aliasing/mutability error.
let instructions = self.inserter.function.dfg[block].instructions().to_vec();
for instruction in instructions.iter() {
self.push_instruction(*instruction);
if self.is_no_predicate(no_predicates, instruction) {
// disable side effect for no_predicate functions
let one = self
.inserter
.function
.dfg
.make_constant(FieldElement::one(), Type::unsigned(1));
self.insert_instruction_with_typevars(
Instruction::EnableSideEffectsIf { condition: one },
None,
im::Vector::new(),
);
self.push_instruction(*instruction);
self.insert_current_side_effects_enabled();
} else {
self.push_instruction(*instruction);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[package]
name = "regression_unsafe_no_predicates"
type = "bin"
authors = [""]
[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x = 239
nest = false
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
fn main(x: u8, nest: bool) {
if nest {
let foo = unsafe_assert([x]);
assert(foo != 0);
}
}

#[no_predicates]
pub fn unsafe_assert<let N: u32>(msg: [u8; N]) -> u8 {
let block = unsafe {
get_block(msg)
};
verify_block(msg, block);
block[0]
}

unconstrained fn get_block<let N: u32>(msg: [u8; N]) -> [u8; 2] {
let mut block: [u8; 2] = [0; 2];
block[0] = msg[0];
block
}

fn verify_block<let N: u32>(msg: [u8; N], block: [u8; 2]) {
assert_eq(block[0], msg[0]);
}

0 comments on commit fc74c55

Please sign in to comment.