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.

Sign up for GitHub

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

Handle out-of-bound errors in CSE (#471) #673

Merged
merged 10 commits into from
Jan 31, 2023
+63 −28
Merged

Handle out-of-bound errors in CSE (#471) #673

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2815bbf
Handle out-of-bound errors in CSE
guipublic Jan 20, 2023
77994a7
Merge branch 'master' into gd/issue_471
guipublic Jan 20, 2023
239b43a
Merge branch 'master' into gd/issue_471
guipublic Jan 23, 2023
abbb690
Code review
guipublic Jan 23, 2023
181ca40
Code review - removes the CseAction error
guipublic Jan 23, 2023
36cd417
Code review
guipublic Jan 31, 2023
6ea2263
Merge branch 'master' into gd/issue_471
guipublic Jan 31, 2023
407cf84
fix clippy error
guipublic Jan 31, 2023
eb26f73
Merge branch 'master' into gd/issue_471
guipublic Jan 31, 2023
1fe847d
Merge branch 'master' into gd/issue_471
guipublic Jan 31, 2023
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
71 changes: 53 additions & 18 deletions crates/noirc_evaluator/src/ssa/anchor.rs
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -230,17 +230,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 @@ -278,13 +278,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 @@ -293,7 +293,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 @@ -307,14 +307,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