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: Optimize array sets in if conditions (alternate version) #4716

Merged
merged 21 commits into from
May 3, 2024
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ impl<'block> BrilligBlock<'block> {
destination_variable,
);
}
Instruction::ArraySet { array, index, value, .. } => {
Instruction::ArraySet { array, index, value, mutable: _ } => {
let source_variable = self.convert_ssa_value(*array, dfg);
let index_register = self.convert_ssa_single_addr_value(*index, dfg);
let value_variable = self.convert_ssa_value(*value, dfg);
Expand Down Expand Up @@ -712,6 +712,9 @@ impl<'block> BrilligBlock<'block> {
Instruction::EnableSideEffects { .. } => {
todo!("enable_side_effects not supported by brillig")
}
Instruction::IfElse { .. } => {
unreachable!("IfElse instructions should not be possible in brillig")
}
};

let dead_variables = self
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 @@ -59,6 +59,7 @@ pub(crate) fn optimize_into_acir(
.run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts:")
// Run mem2reg once more with the flattened CFG to catch any remaining loads/stores
.run_pass(Ssa::mem2reg, "After Mem2Reg:")
.run_pass(Ssa::remove_if_else, "After Remove IfElse:")
// Run the inlining pass again to handle functions with `InlineType::NoPredicates`.
// Before flattening is run, we treat functions marked with the `InlineType::NoPredicates` as an entry point.
.run_pass(Ssa::inline_functions_with_no_predicates, "After Inlining:")
Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,9 @@ impl<'a> Context<'a> {
assert_message.clone(),
)?;
}
Instruction::IfElse { .. } => {
unreachable!("IfElse instruction remaining in acir-gen")
}
}

self.acir_context.set_call_stack(CallStack::new());
Expand Down Expand Up @@ -1009,6 +1012,7 @@ impl<'a> Context<'a> {
});
}
};

if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) {
// Report the error if side effects are enabled.
if index >= array_size {
Expand Down
23 changes: 22 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,10 @@ impl<'dfg> InsertInstructionResult<'dfg> {
match self {
InsertInstructionResult::SimplifiedTo(value) => *value,
InsertInstructionResult::SimplifiedToMultiple(values) => values[0],
InsertInstructionResult::Results(_, results) => results[0],
InsertInstructionResult::Results(_, results) => {
assert_eq!(results.len(), 1);
results[0]
}
InsertInstructionResult::InstructionRemoved => {
panic!("Instruction was removed, no results")
}
Expand Down Expand Up @@ -583,6 +586,24 @@ impl<'dfg> InsertInstructionResult<'dfg> {
}
}

impl<'dfg> std::ops::Index<usize> for InsertInstructionResult<'dfg> {
type Output = ValueId;

fn index(&self, index: usize) -> &Self::Output {
match self {
InsertInstructionResult::Results(_, results) => &results[index],
InsertInstructionResult::SimplifiedTo(result) => {
assert_eq!(index, 0);
result
}
InsertInstructionResult::SimplifiedToMultiple(results) => &results[index],
InsertInstructionResult::InstructionRemoved => {
panic!("Cannot index into InsertInstructionResult::InstructionRemoved")
}
}
}
}

#[cfg(test)]
mod tests {
use super::DataFlowGraph;
Expand Down
63 changes: 61 additions & 2 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use acvm::{acir::BlackBoxFunc, FieldElement};
use iter_extended::vecmap;

use crate::ssa::opt::flatten_cfg::value_merger::ValueMerger;

use super::{
basic_block::BasicBlockId,
dfg::{CallStack, DataFlowGraph},
Expand Down Expand Up @@ -206,6 +208,14 @@ pub(crate) enum Instruction {
/// implemented via reference counting. In ACIR code this is done with im::Vector and these
/// DecrementRc instructions are ignored.
DecrementRc { value: ValueId },

/// Merge two values returned from opposite branches of a conditional into one.
IfElse {
then_condition: ValueId,
then_value: ValueId,
else_condition: ValueId,
else_value: ValueId,
},
}

impl Instruction {
Expand All @@ -219,10 +229,12 @@ impl Instruction {
match self {
Instruction::Binary(binary) => binary.result_type(),
Instruction::Cast(_, typ) => InstructionResultType::Known(typ.clone()),
Instruction::Not(value) | Instruction::Truncate { value, .. } => {
Instruction::Not(value)
| Instruction::Truncate { value, .. }
| Instruction::ArraySet { array: value, .. }
| Instruction::IfElse { then_value: value, .. } => {
InstructionResultType::Operand(*value)
}
Instruction::ArraySet { array, .. } => InstructionResultType::Operand(*array),
Instruction::Constrain(..)
| Instruction::Store { .. }
| Instruction::IncrementRc { .. }
Expand Down Expand Up @@ -270,6 +282,7 @@ impl Instruction {
| Cast(_, _)
| Not(_)
| Truncate { .. }
| IfElse { .. }
| ArrayGet { .. }
| ArraySet { .. } => !self.requires_acir_gen_predicate(dfg),
}
Expand All @@ -295,6 +308,7 @@ impl Instruction {
| Allocate
| Load { .. }
| ArrayGet { .. }
| IfElse { .. }
| ArraySet { .. } => true,

Constrain(..)
Expand Down Expand Up @@ -349,6 +363,7 @@ impl Instruction {
| Instruction::Allocate
| Instruction::Load { .. }
| Instruction::Store { .. }
| Instruction::IfElse { .. }
| Instruction::IncrementRc { .. }
| Instruction::DecrementRc { .. } => false,
}
Expand Down Expand Up @@ -416,6 +431,14 @@ impl Instruction {
assert_message: assert_message.clone(),
}
}
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
Instruction::IfElse {
then_condition: f(*then_condition),
then_value: f(*then_value),
else_condition: f(*else_condition),
else_value: f(*else_value),
}
}
}
}

Expand Down Expand Up @@ -473,6 +496,12 @@ impl Instruction {
| Instruction::RangeCheck { value, .. } => {
f(*value);
}
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
f(*then_condition);
f(*then_value);
f(*else_condition);
f(*else_value);
}
}
}

Expand Down Expand Up @@ -624,6 +653,36 @@ impl Instruction {
None
}
}
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
let typ = dfg.type_of_value(*then_value);

if let Some(constant) = dfg.get_numeric_constant(*then_condition) {
if constant.is_one() {
return SimplifiedTo(*then_value);
} else if constant.is_zero() {
return SimplifiedTo(*else_value);
}
}

if matches!(&typ, Type::Numeric(_)) {
let then_condition = *then_condition;
let then_value = *then_value;
let else_condition = *else_condition;
let else_value = *else_value;

let result = ValueMerger::merge_numeric_values(
dfg,
block,
then_condition,
else_condition,
then_value,
else_value,
);
SimplifiedTo(result)
} else {
None
}
}
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,9 @@ fn simplify_slice_push_back(
slice_sizes.insert(set_last_slice_value, slice_size / element_size);
slice_sizes.insert(new_slice, slice_size / element_size);

let mut value_merger = ValueMerger::new(dfg, block, &mut slice_sizes);
let unknown = &mut HashMap::default();
let mut value_merger = ValueMerger::new(dfg, block, &mut slice_sizes, unknown, None);

let new_slice = value_merger.merge_values(
len_not_equals_capacity,
len_equals_capacity,
Expand Down
12 changes: 11 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ fn display_instruction_inner(
let index = show(*index);
let value = show(*value);
let mutable = if *mutable { " mut" } else { "" };
writeln!(f, "array_set{mutable} {array}, index {index}, value {value}",)
writeln!(f, "array_set{mutable} {array}, index {index}, value {value}")
}
Instruction::IncrementRc { value } => {
writeln!(f, "inc_rc {}", show(*value))
Expand All @@ -195,6 +195,16 @@ fn display_instruction_inner(
Instruction::RangeCheck { value, max_bit_size, .. } => {
writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,)
}
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
let then_condition = show(*then_condition);
let then_value = show(*then_value);
let else_condition = show(*else_condition);
let else_value = show(*else_value);
writeln!(
f,
"if {then_condition} then {then_value} else if {else_condition} then {else_value}"
)
}
}
}

Expand Down
Loading
Loading