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 2 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
26 changes: 24 additions & 2 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 All @@ -18,6 +20,7 @@ pub enum CseAction {
ReplaceWith(NodeId),
Remove(NodeId),
Keep,
Error(RuntimeErrorKind),
}

/// A list of instructions with the same Operation variant
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
jfecher marked this conversation as resolved.
Show resolved Hide resolved
} else {
prev_list.push_front(MemItem::NonConst(id));
}
Ok(())
}

pub fn find_similar_mem_instruction(
Expand Down Expand Up @@ -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() {
jfecher marked this conversation as resolved.
Show resolved Hide resolved
return Some(CseAction::Error(RuntimeErrorKind::ArrayOutOfBounds {
kevaundray marked this conversation as resolved.
Show resolved Hide resolved
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);
Expand Down
19 changes: 10 additions & 9 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 @@ -232,22 +232,23 @@ 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) => {
*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) {
*modified = true;
new_list.remove(id);
}
}
CseAction::Error(err) => return Err(err.into()),
}
}
Operation::Phi { block_args, .. } => {
Expand Down Expand Up @@ -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)?;
}
}
}
Expand All @@ -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)?;
}
}
}
Expand All @@ -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;
}

Expand Down