Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: simplify simple conditionals for brillig #7205

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
6794e3e
simplify simple conditionals for brillig
guipublic Jan 28, 2025
110698d
Merge branch 'master' into gd/issue_6394
guipublic Jan 28, 2025
d341815
fix the manual inline process
guipublic Jan 29, 2025
f3d3fea
Merge branch 'master' into gd/issue_6394
guipublic Jan 29, 2025
1757bc3
another fix to the manual inline
guipublic Jan 29, 2025
d3a918e
Merge branch 'master' into gd/issue_6394
guipublic Jan 29, 2025
6946684
remove debug statement
guipublic Jan 29, 2025
39bc8e6
Add support for IfElse instruction in Brillig
guipublic Jan 29, 2025
da8a8a1
Merge branch 'master' into gd/issue_6394
guipublic Jan 29, 2025
1c91dba
map values in follow-up blocks
guipublic Jan 31, 2025
f24aac7
fix clippy
guipublic Jan 31, 2025
b773bd2
Merge branch 'master' into gd/issue_6394
guipublic Jan 31, 2025
77763b6
Replace mapped instruction instead of inserting a new one
guipublic Jan 31, 2025
56ef385
Merge branch 'master' into gd/issue_6394
guipublic Jan 31, 2025
9fcf348
Add support for arrays and vectors to IfElse in Brillig
guipublic Jan 31, 2025
b79d1da
Merge branch 'master' into gd/issue_6394
guipublic Jan 31, 2025
b86be54
Merge branch 'master' into gd/issue_6394
guipublic Feb 3, 2025
06d26db
map blocks only once
guipublic Feb 3, 2025
7a5ed52
map blocks using post order
guipublic Feb 3, 2025
90aa52c
Use one flattening context per conditional
guipublic Feb 3, 2025
8c7f5b9
Merge branch 'master' into gd/issue_6394
guipublic Feb 3, 2025
b38e806
Merge branch 'master' into gd/issue_6394
guipublic Feb 4, 2025
e4b86df
Merge branch 'master' into gd/issue_6394
guipublic Feb 4, 2025
e10a64b
Merge branch 'master' into gd/issue_6394
guipublic Feb 5, 2025
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
62 changes: 59 additions & 3 deletions compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::brillig::brillig_ir::artifact::Label;
use crate::brillig::brillig_ir::brillig_variable::{
type_to_heap_value_type, BrilligArray, BrilligVariable, SingleAddrVariable,
type_to_heap_value_type, BrilligArray, BrilligVariable, BrilligVector, SingleAddrVariable,
};

use crate::brillig::brillig_ir::registers::RegisterAllocator;
Expand Down Expand Up @@ -853,8 +853,64 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
Instruction::EnableSideEffectsIf { .. } => {
unreachable!("enable_side_effects not supported by brillig")
}
Instruction::IfElse { .. } => {
unreachable!("IfElse instructions should not be possible in brillig")
Instruction::IfElse { then_condition, then_value, else_condition: _, else_value } => {
let then_condition = self.convert_ssa_single_addr_value(*then_condition, dfg);
let then_value = self.convert_ssa_value(*then_value, dfg);
let else_value = self.convert_ssa_value(*else_value, dfg);
let result = self.variables.define_variable(
self.function_context,
self.brillig_context,
dfg.instruction_results(instruction_id)[0],
dfg,
);
match (then_value, else_value) {
(
BrilligVariable::SingleAddr(then_address),
BrilligVariable::SingleAddr(else_address),
) => {
self.brillig_context.conditional_move_instruction(
then_condition,
then_address,
else_address,
result.extract_single_addr(),
);
}
(
BrilligVariable::BrilligArray(then_array),
BrilligVariable::BrilligArray(else_array),
) => {
// Pointer to the array which result from the if-else
let pointer = self.brillig_context.allocate_register();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to allocate another register here? Can we just use the result from above and result.extract_register()?

self.brillig_context.conditional_move_instruction(
then_condition,
SingleAddrVariable::new_usize(then_array.pointer),
SingleAddrVariable::new_usize(else_array.pointer),
SingleAddrVariable::new_usize(pointer),
);
let if_else_array = BrilligArray { pointer, size: then_array.size };
// Copy the if-else array to the result
self.brillig_context
.call_array_copy_procedure(if_else_array, result.extract_array());
}
(
BrilligVariable::BrilligVector(then_vector),
BrilligVariable::BrilligVector(else_vector),
) => {
// Pointer to the vector which result from the if-else
let pointer = self.brillig_context.allocate_register();
self.brillig_context.conditional_move_instruction(
then_condition,
SingleAddrVariable::new_usize(then_vector.pointer),
SingleAddrVariable::new_usize(else_vector.pointer),
SingleAddrVariable::new_usize(pointer),
);
let if_else_vector = BrilligVector { pointer };
// Copy the if-else vector to the result
self.brillig_context
.call_vector_copy_procedure(if_else_vector, result.extract_vector());
}
_ => unreachable!("ICE - then and else values must have the same type"),
}
}
Instruction::MakeArray { elements: array, typ } => {
let value_id = dfg.instruction_results(instruction_id)[0];
Expand Down
18 changes: 18 additions & 0 deletions compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,24 @@
debug_println!(self.enable_debug_trace, " MOV {}, {}", destination, source);
}

/// Emits a `conditional mov` instruction.
pub(crate) fn conditional_mov_instruction(
&self,
destination: MemoryAddress,
source_a: MemoryAddress,
source_b: MemoryAddress,
condition: MemoryAddress,
) {
debug_println!(
self.enable_debug_trace,
" {} = CONDITIONAL MOV {} then {}, else {}",
destination,
condition,
source_a,
source_b
);
}

/// Emits a `cast` instruction.
pub(crate) fn cast_instruction(
&self,
Expand Down Expand Up @@ -164,7 +182,7 @@
result_pointer: MemoryAddress,
constant: F,
) {
debug_println!(self.enable_debug_trace, " ICONST {} = {}", result_pointer, constant);

Check warning on line 185 in compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (ICONST)
}

/// Processes a not instruction. Append with "_" as this is a high-level instruction.
Expand Down
22 changes: 22 additions & 0 deletions compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,28 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
);
}

/// Insert a conditional move instruction
pub(crate) fn conditional_move_instruction(
&mut self,
condition: SingleAddrVariable,
then_address: SingleAddrVariable,
else_address: SingleAddrVariable,
destination: SingleAddrVariable,
) {
self.debug_show.conditional_mov_instruction(
destination.address,
then_address.address,
else_address.address,
condition.address,
);
self.push_opcode(BrilligOpcode::ConditionalMov {
destination: destination.address,
source_a: then_address.address,
source_b: else_address.address,
condition: condition.address,
});
}

fn binary(
&mut self,
lhs: SingleAddrVariable,
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result<Ss
.run_pass(Ssa::remove_if_else, "Remove IfElse")
.run_pass(Ssa::purity_analysis, "Purity Analysis (2nd)")
.run_pass(Ssa::fold_constants, "Constant Folding")
.run_pass(Ssa::flatten_basic_conditionals, "Simplify conditionals for unconstrained")
.run_pass(Ssa::remove_enable_side_effects, "EnableSideEffectsIf removal")
.run_pass(Ssa::fold_constants_using_constraints, "Constraint Folding")
.run_pass(Ssa::make_constrain_not_equal_instructions, "Adding constrain not equal")
Expand Down
12 changes: 8 additions & 4 deletions compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,10 +239,9 @@ impl DataFlowGraph {
instruction,
Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. }
),
RuntimeType::Brillig(_) => !matches!(
instruction,
Instruction::EnableSideEffectsIf { .. } | Instruction::IfElse { .. }
),
RuntimeType::Brillig(_) => {
!matches!(instruction, Instruction::EnableSideEffectsIf { .. })
}
}
}

Expand Down Expand Up @@ -372,6 +371,11 @@ impl DataFlowGraph {
}
}

/// Replace an existing instruction with a new one.
pub(crate) fn set_instruction(&mut self, id: InstructionId, instruction: Instruction) {
self.instructions[id] = instruction;
}

/// Set the value of value_to_replace to refer to the value referred to by new_value.
///
/// This is the preferred method to call for optimizations simplifying
Expand Down
24 changes: 24 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ impl<'f> FunctionInserter<'f> {
(instruction, self.function.dfg.get_instruction_call_stack_id(id))
}

/// Get an instruction, map all its values, and replace it with the resolved instruction.
pub(crate) fn map_instruction_in_place(&mut self, id: InstructionId) {
let mut instruction = self.function.dfg[id].clone();
instruction.map_values_mut(|id| self.resolve(id));
self.function.dfg.set_instruction(id, instruction);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

nit just to separate the } from the next comment

/// Maps a terminator in place, replacing any ValueId in the terminator with the
/// resolved version of that value id from this FunctionInserter's internal value mapping.
pub(crate) fn map_terminator_in_place(&mut self, block: BasicBlockId) {
Expand Down Expand Up @@ -251,4 +257,22 @@ impl<'f> FunctionInserter<'f> {
self.values.entry(*param).or_insert(*new_param);
}
}

/// Merge the internal mapping into the given mapping
/// The merge is guaranteed to be coherent because ambiguous cases are prevented
pub(crate) fn extract_mapping(&self, mapping: &mut HashMap<ValueId, ValueId>) {
for (k, v) in &self.values {
if mapping.contains_key(k) {
unreachable!("cannot merge key");
}
if mapping.contains_key(v) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to check contains_key for the value here?

unreachable!("cannot merge value");
}
mapping.insert(*k, *v);
}
}

pub(crate) fn set_mapping(&mut self, mapping: HashMap<ValueId, ValueId>) {
self.values = mapping;
}
}
Loading
Loading