Skip to content

Commit

Permalink
Handle out-of-bound errors in CSE (#471) (#673)
Browse files Browse the repository at this point in the history 
* Handle out-of-bound errors in CSE

* Code review

* Code review - removes the CseAction error

* Code review

* fix clippy error
  • Loading branch information
@guipublic
guipublic authored Jan 31, 2023
1 parent a966279 commit 4872e91
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 28 deletions.
71 changes: 53 additions & 18 deletions crates/noirc_evaluator/src/ssa/anchor.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use std::collections::{HashMap, VecDeque};

use crate::errors::{RuntimeError, RuntimeErrorKind};

use super::{
context::SsaContext,
mem::ArrayId,
Expand Down Expand Up @@ -89,8 +91,8 @@ impl Anchor {
None
}

fn get_mem_map(&self, a: ArrayId) -> &Vec<VecDeque<(usize, NodeId)>> {
&self.mem_map[&a]
fn get_mem_map(&self, a: &ArrayId) -> &Vec<VecDeque<(usize, NodeId)>> {
&self.mem_map[a]
}

pub fn get_mem_all(&self, a: ArrayId) -> &VecDeque<MemItem> {
Expand All @@ -105,7 +107,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);
Expand Down Expand Up @@ -134,25 +140,27 @@ impl Anchor {
len
}
};
self.mem_map.get_mut(&array_id).unwrap()[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));
}
Ok(())
}

pub fn find_similar_mem_instruction(
&self,
ctx: &SsaContext,
op: &Operation,
prev_ins: &VecDeque<MemItem>,
) -> CseAction {
) -> Result<CseAction, RuntimeErrorKind> {
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) {
Expand All @@ -168,41 +176,68 @@ impl Anchor {
ctx: &SsaContext,
item: &MemItem,
op: &Operation,
) -> Option<CseAction> {
) -> Result<Option<CseAction>, RuntimeErrorKind> {
let (array_id, index, is_load) = Anchor::get_mem_op(op);
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;
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() {
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)),
}
}
}

//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,
Expand Down
20 changes: 10 additions & 10 deletions crates/noirc_evaluator/src/ssa/optim.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,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);
Expand All @@ -240,17 +240,17 @@ 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);
anchor.push_mem_instruction(ctx, *ins_id)?;
new_list.push(*ins_id)
}
CseAction::ReplaceWith(new_id) => {
*modified = true;
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) {
Expand Down Expand Up @@ -288,13 +288,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)?;
}
}
}
Expand All @@ -303,7 +303,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)?;
}
}
}
Expand All @@ -317,14 +317,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;
}

Expand Down

0 comments on commit 4872e91

Please sign in to comment.