From 2815bbfea22422c4ddf362c16aa68044061d918a Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 20 Jan 2023 13:49:15 +0000 Subject: [PATCH 1/5] Handle out-of-bound errors in CSE --- crates/noirc_evaluator/src/ssa/anchor.rs | 26 ++++++++++++++++++++++-- crates/noirc_evaluator/src/ssa/optim.rs | 19 +++++++++-------- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/anchor.rs b/crates/noirc_evaluator/src/ssa/anchor.rs index 17b10451fbe..407ccb59f53 100644 --- a/crates/noirc_evaluator/src/ssa/anchor.rs +++ b/crates/noirc_evaluator/src/ssa/anchor.rs @@ -1,5 +1,7 @@ use std::collections::{HashMap, VecDeque}; +use crate::errors::{RuntimeError, RuntimeErrorKind}; + use super::{ context::SsaContext, mem::ArrayId, @@ -18,6 +20,7 @@ pub enum CseAction { ReplaceWith(NodeId), Remove(NodeId), Keep, + Error(RuntimeErrorKind), } /// A list of instructions with the same Operation variant @@ -105,7 +108,11 @@ impl Anchor { } } - pub fn push_mem_instruction(&mut self, ctx: &SsaContext, id: NodeId) { + pub fn push_mem_instruction( + &mut self, + ctx: &SsaContext, + id: NodeId, + ) -> Result<(), RuntimeError> { let ins = ctx.get_instruction(id); let (array_id, index, is_load) = Anchor::get_mem_op(&ins.operation); self.use_array(array_id, ctx.mem[array_id].len as usize); @@ -134,10 +141,19 @@ impl Anchor { len } }; - self.mem_map.get_mut(&array_id).unwrap()[mem_idx].push_front((item_pos, id)); + let arr = self.mem_map.get_mut(&array_id).unwrap(); + if mem_idx > arr.len() { + return Err(RuntimeErrorKind::ArrayOutOfBounds { + index: mem_idx as u128, + bound: arr.len() as u128, + } + .into()); + } + arr[mem_idx].push_front((item_pos, id)); } else { prev_list.push_front(MemItem::NonConst(id)); } + Ok(()) } pub fn find_similar_mem_instruction( @@ -175,6 +191,12 @@ impl Anchor { MemItem::Const(p) | MemItem::ConstLoad(p) => { let a = self.get_mem_map(array_id); let b_idx = b_value.to_u128() as usize; + if b_idx >= a.len() { + return Some(CseAction::Error(RuntimeErrorKind::ArrayOutOfBounds { + index: b_idx as u128, + bound: a.len() as u128, + })); + } for (pos, id) in &a[b_idx] { if pos == p { let action = Anchor::match_mem_id(ctx, *id, index, is_load); diff --git a/crates/noirc_evaluator/src/ssa/optim.rs b/crates/noirc_evaluator/src/ssa/optim.rs index 72604d58a18..e4a4b74305f 100644 --- a/crates/noirc_evaluator/src/ssa/optim.rs +++ b/crates/noirc_evaluator/src/ssa/optim.rs @@ -204,11 +204,11 @@ fn cse_block_with_anchor( //No CSE for arrays because they are not in SSA form //We could improve this in future by checking if the arrays are immutable or not modified in-between let id = ctx.get_dummy_load(a); - anchor.push_mem_instruction(ctx, id); + anchor.push_mem_instruction(ctx, id)?; if let ObjectType::Pointer(a) = ctx.get_object_type(binary.rhs) { let id = ctx.get_dummy_load(a); - anchor.push_mem_instruction(ctx, id); + anchor.push_mem_instruction(ctx, id)?; } new_list.push(*ins_id); @@ -232,7 +232,7 @@ fn cse_block_with_anchor( let prev_ins = anchor.get_mem_all(*x); match anchor.find_similar_mem_instruction(ctx, &operator, prev_ins) { CseAction::Keep => { - anchor.push_mem_instruction(ctx, *ins_id); + anchor.push_mem_instruction(ctx, *ins_id)?; new_list.push(*ins_id) } CseAction::ReplaceWith(new_id) => { @@ -240,7 +240,7 @@ fn cse_block_with_anchor( new_mark = Mark::ReplaceWith(new_id); } CseAction::Remove(id_to_remove) => { - anchor.push_mem_instruction(ctx, *ins_id); + anchor.push_mem_instruction(ctx, *ins_id)?; new_list.push(*ins_id); // TODO if not found, it should be removed from other blocks; we could keep a list of instructions to remove if let Some(id) = new_list.iter().position(|x| *x == id_to_remove) { @@ -248,6 +248,7 @@ fn cse_block_with_anchor( new_list.remove(id); } } + CseAction::Error(err) => return Err(err.into()), } } Operation::Phi { block_args, .. } => { @@ -278,13 +279,13 @@ fn cse_block_with_anchor( //Add dummy store for functions that modify arrays for a in returned_arrays { let id = ctx.get_dummy_store(a.0); - anchor.push_mem_instruction(ctx, id); + anchor.push_mem_instruction(ctx, id)?; } if let Some(f) = ctx.try_get_ssafunc(*func) { for typ in &f.result_types { if let ObjectType::Pointer(a) = typ { let id = ctx.get_dummy_store(*a); - anchor.push_mem_instruction(ctx, id); + anchor.push_mem_instruction(ctx, id)?; } } } @@ -293,7 +294,7 @@ fn cse_block_with_anchor( if let Some(obj) = ctx.try_get_node(*arg) { if let ObjectType::Pointer(a) = obj.get_type() { let id = ctx.get_dummy_load(a); - anchor.push_mem_instruction(ctx, id); + anchor.push_mem_instruction(ctx, id)?; } } } @@ -307,14 +308,14 @@ fn cse_block_with_anchor( if let Some(obj) = ctx.try_get_node(*arg) { if let ObjectType::Pointer(a) = obj.get_type() { let id = ctx.get_dummy_load(a); - anchor.push_mem_instruction(ctx, id); + anchor.push_mem_instruction(ctx, id)?; activate_cse = false; } } } if let ObjectType::Pointer(a) = ins.res_type { let id = ctx.get_dummy_store(a); - anchor.push_mem_instruction(ctx, id); + anchor.push_mem_instruction(ctx, id)?; activate_cse = false; } From abbb6903eb79fbdb13224b0a5fd3cca8f6fa6778 Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 23 Jan 2023 09:26:33 +0000 Subject: [PATCH 2/5] Code review --- crates/noirc_evaluator/src/ssa/anchor.rs | 28 ++++++++++++------------ crates/noirc_evaluator/src/ssa/optim.rs | 2 +- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/anchor.rs b/crates/noirc_evaluator/src/ssa/anchor.rs index 407ccb59f53..b94b8634c45 100644 --- a/crates/noirc_evaluator/src/ssa/anchor.rs +++ b/crates/noirc_evaluator/src/ssa/anchor.rs @@ -161,14 +161,14 @@ impl Anchor { ctx: &SsaContext, op: &Operation, prev_ins: &VecDeque, - ) -> CseAction { + ) -> Result { for iter in prev_ins.iter() { - if let Some(action) = self.match_mem_item(ctx, iter, op) { - return action; + if let Some(action) = self.match_mem_item(ctx, iter, op)? { + return Ok(action); } } - CseAction::Keep + Ok(CseAction::Keep) } fn get_mem_op(op: &Operation) -> (ArrayId, NodeId, bool) { @@ -184,7 +184,7 @@ impl Anchor { ctx: &SsaContext, item: &MemItem, op: &Operation, - ) -> Option { + ) -> Result, RuntimeErrorKind> { let (array_id, index, is_load) = Anchor::get_mem_op(op); if let Some(b_value) = ctx.get_as_constant(index) { match item { @@ -192,35 +192,35 @@ impl Anchor { let a = self.get_mem_map(array_id); let b_idx = b_value.to_u128() as usize; if b_idx >= a.len() { - return Some(CseAction::Error(RuntimeErrorKind::ArrayOutOfBounds { + return Ok(Some(CseAction::Error(RuntimeErrorKind::ArrayOutOfBounds { index: b_idx as u128, bound: a.len() as u128, - })); + }))); } for (pos, id) in &a[b_idx] { if pos == p { let action = Anchor::match_mem_id(ctx, *id, index, is_load); if action.is_some() { - return action; + return Ok(action); } } } - None + Ok(None) } - MemItem::NonConst(id) => Anchor::match_mem_id(ctx, *id, index, is_load), + MemItem::NonConst(id) => Ok(Anchor::match_mem_id(ctx, *id, index, is_load)), } } else { match item { - MemItem::Const(_) => Some(CseAction::Keep), + MemItem::Const(_) => Ok(Some(CseAction::Keep)), MemItem::ConstLoad(_) => { if is_load { - None + Ok(None) } else { - Some(CseAction::Keep) + Ok(Some(CseAction::Keep)) } } - MemItem::NonConst(id) => Anchor::match_mem_id(ctx, *id, index, is_load), + MemItem::NonConst(id) => Ok(Anchor::match_mem_id(ctx, *id, index, is_load)), } } } diff --git a/crates/noirc_evaluator/src/ssa/optim.rs b/crates/noirc_evaluator/src/ssa/optim.rs index e4a4b74305f..dac019e07de 100644 --- a/crates/noirc_evaluator/src/ssa/optim.rs +++ b/crates/noirc_evaluator/src/ssa/optim.rs @@ -230,7 +230,7 @@ fn cse_block_with_anchor( } anchor.use_array(*x, ctx.mem[*x].len as usize); let prev_ins = anchor.get_mem_all(*x); - match anchor.find_similar_mem_instruction(ctx, &operator, prev_ins) { + match anchor.find_similar_mem_instruction(ctx, &operator, prev_ins)? { CseAction::Keep => { anchor.push_mem_instruction(ctx, *ins_id)?; new_list.push(*ins_id) From 181ca403280cfa0d61b54152ac0f9e5cfe4cb233 Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 23 Jan 2023 09:29:38 +0000 Subject: [PATCH 3/5] Code review - removes the CseAction error --- crates/noirc_evaluator/src/ssa/anchor.rs | 5 ++--- crates/noirc_evaluator/src/ssa/optim.rs | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/anchor.rs b/crates/noirc_evaluator/src/ssa/anchor.rs index b94b8634c45..68c5fcc4c27 100644 --- a/crates/noirc_evaluator/src/ssa/anchor.rs +++ b/crates/noirc_evaluator/src/ssa/anchor.rs @@ -20,7 +20,6 @@ pub enum CseAction { ReplaceWith(NodeId), Remove(NodeId), Keep, - Error(RuntimeErrorKind), } /// A list of instructions with the same Operation variant @@ -192,10 +191,10 @@ impl Anchor { let a = self.get_mem_map(array_id); let b_idx = b_value.to_u128() as usize; if b_idx >= a.len() { - return Ok(Some(CseAction::Error(RuntimeErrorKind::ArrayOutOfBounds { + return Err(RuntimeErrorKind::ArrayOutOfBounds { index: b_idx as u128, bound: a.len() as u128, - }))); + }); } for (pos, id) in &a[b_idx] { if pos == p { diff --git a/crates/noirc_evaluator/src/ssa/optim.rs b/crates/noirc_evaluator/src/ssa/optim.rs index dac019e07de..726d0d70914 100644 --- a/crates/noirc_evaluator/src/ssa/optim.rs +++ b/crates/noirc_evaluator/src/ssa/optim.rs @@ -248,7 +248,6 @@ fn cse_block_with_anchor( new_list.remove(id); } } - CseAction::Error(err) => return Err(err.into()), } } Operation::Phi { block_args, .. } => { From 36cd417ad231e33014cfa380d23c02b00659cbfb Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 31 Jan 2023 13:37:44 +0000 Subject: [PATCH 4/5] Code review --- crates/noirc_evaluator/src/ssa/anchor.rs | 52 +++++++++++++++--------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/anchor.rs b/crates/noirc_evaluator/src/ssa/anchor.rs index 68c5fcc4c27..fbb258ec37c 100644 --- a/crates/noirc_evaluator/src/ssa/anchor.rs +++ b/crates/noirc_evaluator/src/ssa/anchor.rs @@ -91,8 +91,8 @@ impl Anchor { None } - fn get_mem_map(&self, a: ArrayId) -> &Vec> { - &self.mem_map[&a] + fn get_mem_map(&self, a: &ArrayId) -> &Vec> { + &self.mem_map[a] } pub fn get_mem_all(&self, a: ArrayId) -> &VecDeque { @@ -140,15 +140,8 @@ impl Anchor { len } }; - let arr = self.mem_map.get_mut(&array_id).unwrap(); - if mem_idx > arr.len() { - return Err(RuntimeErrorKind::ArrayOutOfBounds { - index: mem_idx as u128, - bound: arr.len() as u128, - } - .into()); - } - arr[mem_idx].push_front((item_pos, id)); + let anchor_list = self.get_anchor_list_mut(&array_id, mem_idx)?; + anchor_list.push_front((item_pos, id)); } else { prev_list.push_front(MemItem::NonConst(id)); } @@ -188,15 +181,9 @@ impl Anchor { if let Some(b_value) = ctx.get_as_constant(index) { match item { MemItem::Const(p) | MemItem::ConstLoad(p) => { - let a = self.get_mem_map(array_id); let b_idx = b_value.to_u128() as usize; - if b_idx >= a.len() { - return Err(RuntimeErrorKind::ArrayOutOfBounds { - index: b_idx as u128, - bound: a.len() as u128, - }); - } - for (pos, id) in &a[b_idx] { + let anchor_list = self.get_anchor_list(&array_id, b_idx)?; + for (pos, id) in anchor_list { if pos == p { let action = Anchor::match_mem_id(ctx, *id, index, is_load); if action.is_some() { @@ -224,6 +211,33 @@ impl Anchor { } } + //Returns the anchor list of memory instructions for the array_id at the provided index + // It issues an out-of-bound error when the list does not exist at this index. + fn get_anchor_list( + &self, + array_id: &ArrayId, + index: usize, + ) -> Result<&VecDeque<(usize, NodeId)>, RuntimeErrorKind> { + let memory_map = self.get_mem_map(array_id); + memory_map.get(index).ok_or(RuntimeErrorKind::ArrayOutOfBounds { + index: index as u128, + bound: memory_map.len() as u128, + }) + } + + //Same as get_anchor_list() but returns a mutable anchor + fn get_anchor_list_mut( + &mut self, + array_id: &ArrayId, + index: usize, + ) -> Result<&mut VecDeque<(usize, NodeId)>, RuntimeErrorKind> { + let memory_map = self.mem_map.get_mut(&array_id).unwrap(); + let len = memory_map.len() as u128; + memory_map + .get_mut(index) + .ok_or(RuntimeErrorKind::ArrayOutOfBounds { index: index as u128, bound: len }) + } + fn match_mem_id( ctx: &SsaContext, a: NodeId, From 407cf84df31bb38be27c4b7ac4e86ca6ea13a34d Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 31 Jan 2023 14:20:23 +0000 Subject: [PATCH 5/5] fix clippy error --- crates/noirc_evaluator/src/ssa/anchor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/anchor.rs b/crates/noirc_evaluator/src/ssa/anchor.rs index fbb258ec37c..c0ab28402ad 100644 --- a/crates/noirc_evaluator/src/ssa/anchor.rs +++ b/crates/noirc_evaluator/src/ssa/anchor.rs @@ -231,7 +231,7 @@ impl Anchor { array_id: &ArrayId, index: usize, ) -> Result<&mut VecDeque<(usize, NodeId)>, RuntimeErrorKind> { - let memory_map = self.mem_map.get_mut(&array_id).unwrap(); + let memory_map = self.mem_map.get_mut(array_id).unwrap(); let len = memory_map.len() as u128; memory_map .get_mut(index)