From 1c98ac601653cb0faa271a00a975b8432d1cd3e7 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 20 Aug 2024 10:40:43 -0500 Subject: [PATCH 1/7] Follow array sets when optimizing array gets --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 73 ++++++++++++++++--- 1 file changed, 64 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 7dcb50762f5..f9be812f629 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -10,6 +10,7 @@ use acvm::{ }, FieldElement, }; +use fxhash::FxHashSet as HashSet; use fxhash::FxHasher; use iter_extended::vecmap; use noirc_frontend::hir_def::types::Type as HirType; @@ -608,16 +609,11 @@ impl Instruction { } } Instruction::ArrayGet { array, index } => { - let array = dfg.get_array_constant(*array); - let index = dfg.get_numeric_constant(*index); - if let (Some((array, _)), Some(index)) = (array, index) { - let index = - index.try_to_u32().expect("Expected array index to fit in u32") as usize; - if index < array.len() { - return SimplifiedTo(array[index]); - } + if let Some(index) = dfg.get_numeric_constant(*index) { + try_optimize_array_get_from_previous_set(dfg, *array, index) + } else { + None } - None } Instruction::ArraySet { array, index, value, .. } => { let array = dfg.get_array_constant(*array); @@ -744,6 +740,65 @@ impl Instruction { } } +/// Given a chain of operations like: +/// v1 = array_set [10, 11, 12], index 1, value: 5 +/// v2 = array_set v1, index 2, value: 6 +/// v3 = array_set v2, index 2, value: 7 +/// v4 = array_get v3, index 1 +/// +/// We want to optimize `v4` to `10`. To do this we need to follow the array value +/// through several array sets. For each array set: +/// - If the index is non-constant we fail the optimization since any index may be changed +/// - If the index is constant and is our target index, we conservatively fail the optimization +/// in case the array_set is disabled from a previous `enable_side_effects_if` and the array get +/// was not. +/// - Otherwise, we check the array value of the array set. +/// - If the array value is constant, we use that array. +/// - If the array value is from a previous array-set, we recur. +fn try_optimize_array_get_from_previous_set(dfg: &mut DataFlowGraph, mut array_id: Id, target_index: FieldElement) -> SimplifyResult { + let mut elements = None; + let mut set_indices = HashSet::default(); + + // Arbitrary number of maximum tries just to prevent this optimization from taking too long. + let max_tries = 5; + for _ in 0 .. max_tries { + match &dfg[array_id] { + Value::Instruction { instruction, .. } => { + match &dfg[*instruction] { + Instruction::ArraySet { array, index, value, .. } => { + if let Some(constant) = dfg.get_numeric_constant(*index) { + if constant == target_index { + return SimplifyResult::None; + } + + set_indices.insert(constant); + array_id = *array; // recur + } else { + return SimplifyResult::None; + } + }, + _ => return SimplifyResult::None, + } + }, + Value::Array { array, typ: _ } => { + elements = Some(array.clone()); + break; + }, + _ => return SimplifyResult::None, + } + } + + if let Some(array) = elements { + if let Some(index) = target_index.try_to_u64() { + let index = index as usize; + if index < array.len() { + return SimplifyResult::SimplifiedTo(array[index]); + } + } + } + SimplifyResult::None +} + pub(crate) type ErrorType = HirType; pub(crate) fn error_selector_from_type(typ: &ErrorType) -> ErrorSelector { From e38c441b7c8e420d32ee676ff8b3c26da3779c87 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 20 Aug 2024 10:46:16 -0500 Subject: [PATCH 2/7] Clippy --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index f9be812f629..9c2b2965be6 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -755,17 +755,21 @@ impl Instruction { /// - Otherwise, we check the array value of the array set. /// - If the array value is constant, we use that array. /// - If the array value is from a previous array-set, we recur. -fn try_optimize_array_get_from_previous_set(dfg: &mut DataFlowGraph, mut array_id: Id, target_index: FieldElement) -> SimplifyResult { +fn try_optimize_array_get_from_previous_set( + dfg: &mut DataFlowGraph, + mut array_id: Id, + target_index: FieldElement, +) -> SimplifyResult { let mut elements = None; let mut set_indices = HashSet::default(); // Arbitrary number of maximum tries just to prevent this optimization from taking too long. let max_tries = 5; - for _ in 0 .. max_tries { + for _ in 0..max_tries { match &dfg[array_id] { Value::Instruction { instruction, .. } => { match &dfg[*instruction] { - Instruction::ArraySet { array, index, value, .. } => { + Instruction::ArraySet { array, index, .. } => { if let Some(constant) = dfg.get_numeric_constant(*index) { if constant == target_index { return SimplifyResult::None; @@ -776,14 +780,14 @@ fn try_optimize_array_get_from_previous_set(dfg: &mut DataFlowGraph, mut array_i } else { return SimplifyResult::None; } - }, + } _ => return SimplifyResult::None, } - }, + } Value::Array { array, typ: _ } => { elements = Some(array.clone()); break; - }, + } _ => return SimplifyResult::None, } } From 07d24cb73725842f6450e3a660262cfb91e911ee Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 20 Aug 2024 11:00:51 -0500 Subject: [PATCH 3/7] Add set optimization --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 9c2b2965be6..a8271163b57 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -769,10 +769,10 @@ fn try_optimize_array_get_from_previous_set( match &dfg[array_id] { Value::Instruction { instruction, .. } => { match &dfg[*instruction] { - Instruction::ArraySet { array, index, .. } => { + Instruction::ArraySet { array, index, value, .. } => { if let Some(constant) = dfg.get_numeric_constant(*index) { if constant == target_index { - return SimplifyResult::None; + return SimplifyResult::SimplifiedTo(*value); } set_indices.insert(constant); From 08af2c4277140a8ad8f31abd3a6792bc498af135 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 20 Aug 2024 11:14:48 -0500 Subject: [PATCH 4/7] Try changing the constant from 5 to 20 --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index a8271163b57..255a3935341 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -10,7 +10,6 @@ use acvm::{ }, FieldElement, }; -use fxhash::FxHashSet as HashSet; use fxhash::FxHasher; use iter_extended::vecmap; use noirc_frontend::hir_def::types::Type as HirType; @@ -761,10 +760,9 @@ fn try_optimize_array_get_from_previous_set( target_index: FieldElement, ) -> SimplifyResult { let mut elements = None; - let mut set_indices = HashSet::default(); // Arbitrary number of maximum tries just to prevent this optimization from taking too long. - let max_tries = 5; + let max_tries = 20; for _ in 0..max_tries { match &dfg[array_id] { Value::Instruction { instruction, .. } => { @@ -775,7 +773,6 @@ fn try_optimize_array_get_from_previous_set( return SimplifyResult::SimplifiedTo(*value); } - set_indices.insert(constant); array_id = *array; // recur } else { return SimplifyResult::None; From 9e151afaf3774d00835c7b79cf3b354d6efc6cc8 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 20 Aug 2024 11:23:11 -0500 Subject: [PATCH 5/7] Revert constant; it had no effect --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 255a3935341..7322d771910 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -762,7 +762,7 @@ fn try_optimize_array_get_from_previous_set( let mut elements = None; // Arbitrary number of maximum tries just to prevent this optimization from taking too long. - let max_tries = 20; + let max_tries = 5; for _ in 0..max_tries { match &dfg[array_id] { Value::Instruction { instruction, .. } => { From c4b20d66d4aef6cac7719a088d8e48946bfad832 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 20 Aug 2024 11:37:21 -0500 Subject: [PATCH 6/7] Reduce nesting --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 7322d771910..bafaccb7c39 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -789,12 +789,10 @@ fn try_optimize_array_get_from_previous_set( } } - if let Some(array) = elements { - if let Some(index) = target_index.try_to_u64() { - let index = index as usize; - if index < array.len() { - return SimplifyResult::SimplifiedTo(array[index]); - } + if let (Some(array), Some(index)) = (elements, target_index.try_to_u64()) { + let index = index as usize; + if index < array.len() { + return SimplifyResult::SimplifiedTo(array[index]); } } SimplifyResult::None From e3f7fcfd9975287255c80f957269c27120588ac6 Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 20 Aug 2024 11:47:12 -0500 Subject: [PATCH 7/7] Update compiler/noirc_evaluator/src/ssa/ir/instruction.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index bafaccb7c39..1b3466c76fa 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -755,7 +755,7 @@ impl Instruction { /// - If the array value is constant, we use that array. /// - If the array value is from a previous array-set, we recur. fn try_optimize_array_get_from_previous_set( - dfg: &mut DataFlowGraph, + dfg: &DataFlowGraph, mut array_id: Id, target_index: FieldElement, ) -> SimplifyResult {