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

Implement Range for OpStack #338

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Implement Range for OpStack #338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b564720
range for opstack
cyberbono3 Nov 1, 2024
9448ce0
WIP tests
cyberbono3 Nov 1, 2024
9020495
all tests pass
cyberbono3 Nov 2, 2024
ddfb293
minor fix
cyberbono3 Nov 2, 2024
db04e40
update
cyberbono3 Dec 12, 2024
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
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,6 @@ specification/cheatsheet.aux
specification/cheatsheet.log
specification/cheatsheet.synctex.gz
triton-vm/proofs/*

# Ignore proptest-regression processor file caused by bug
triton-air/proptest-regressions/table/processor.txt
271 changes: 269 additions & 2 deletions triton-isa/src/op_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ use std::fmt::Display;
use std::fmt::Formatter;
use std::fmt::Result as FmtResult;
use std::num::TryFromIntError;
use std::ops::Index;
use std::ops::IndexMut;
use std::ops::{Index, IndexMut, Range, RangeInclusive};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the wrong way around. 🙂 You've applied item granularity “module”, whereas granularity “item” results in one line per used type. See also here.


use arbitrary::Arbitrary;
use get_size::GetSize;
Expand Down Expand Up @@ -183,13 +182,69 @@ impl Index<usize> for OpStack {
}
}

fn compute_range(start: usize, end: usize, len: usize) -> (usize, usize) {
match (start, end) {
(0, end) if end == len => (0, len),
(0, end) => (len - end, len),
_ => (len - end, len - start),
}
}
Comment on lines +185 to +191
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name does not really describe what the function does. A better name could be flip_range, indicating that the range is mirrored or flipped along the “length” axis. There might be better names yet.

Furthermore, I don't think the special cases are necessary since they all collapse into the general case.

fn flip_range(start: usize, end: usize, len: usize) -> (usize, usize) {
    (len - end, len - start)
}


impl Index<Range<usize>> for OpStack {
type Output = [BFieldElement];

fn index(&self, range: Range<usize>) -> &Self::Output {
let (start, end) = compute_range(range.start, range.end, self.stack.len());
let mut stack = self.stack[start..end].to_vec();
stack.reverse();
Box::leak(Box::new(stack))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a creative way of solving this. 😃 However, I feel uncomfortable with consciously introducing memory leaks.

The approach you take here indicates to me that implementing Index<Range<_>> for OpStack requires refactoring OpStack such that the internal field stack is always in the correct order, for example using a VecDeque. It's probably a good idea to make the field non-public at the same time. Please let me know if this is too much scope creep. 😌

Copy link
Author

@cyberbono3 cyberbono3 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduction of VecDeque sounds a good idea.

pub struct OpStack {
    pub stack: VecDeque<BFieldElement>,

    underflow_io_sequence: Vec<UnderflowIO>,
}

impl OpStack {
    pub fn new(program_digest: Digest) -> Self {
        let mut stack = VecDeque::with_capacity(OpStackElement::COUNT);
        stack.extend(program_digest.values());

        Self {
            stack,
            underflow_io_sequence: vec![],
        }
    }

In this case we do not reverse program_digest and set self.stack[0] is a top of the stack.

Additionally, we can update OpStack.push, OpStack.pop to add new item to the beginning of the stack and remove item from the beginning of the stack, respectively. Vec<BFieldElement> lacks this functionality.

  pub fn push(&mut self, element: BFieldElement) {
        self.stack.push_front(element);
        self.record_underflow_io(UnderflowIO::Write);
    }

    pub fn pop(&mut self) -> Result<BFieldElement> {
        self.record_underflow_io(UnderflowIO::Read);
        self.stack.pop_front().ok_or(OpStackError::TooShallow)
    }

    pub fn insert(&mut self, index: OpStackElement, element: BFieldElement) {
        let insertion_index = usize::from(index);
        self.stack.insert(insertion_index, element);
        self.record_underflow_io(UnderflowIO::Write);
    }

Index<Range<usize>> for OpStack can be implemented like that :

impl Index<Range<usize>> for OpStack {
    type Output = [BFieldElement];

    fn index(&self, range: Range<usize>) -> &Self::Output {
        &self.stack.as_slices().0[range.start..range.end]
    }
}

I wonder if this make sense.

}
}

fn compute_range_inclusive(start: usize, end: usize, len: usize) -> (usize, usize) {
match (start, end) {
(0, end) if end == len - 1 => (0, len),
(0, end) => (len - end - 1, len),
_ => (len - end - 1, len - start),
}
}

impl Index<RangeInclusive<usize>> for OpStack {
type Output = [BFieldElement];

fn index(&self, range: RangeInclusive<usize>) -> &Self::Output {
let (start, end) = compute_range_inclusive(*range.start(), *range.end(), self.stack.len());
let mut stack = self.stack[start..end].to_vec();
stack.reverse();
Box::leak(Box::new(stack))
}
}

impl IndexMut<usize> for OpStack {
fn index_mut(&mut self, index: usize) -> &mut Self::Output {
let top_of_stack = self.len() - 1;
&mut self.stack[top_of_stack - index]
}
}

impl IndexMut<Range<usize>> for OpStack {
fn index_mut(&mut self, range: Range<usize>) -> &mut Self::Output {
let (start, end) = compute_range(range.start, range.end, self.stack.len());
let slice = &mut self.stack[start..end];
slice.reverse();
slice
}
}

impl IndexMut<RangeInclusive<usize>> for OpStack {
fn index_mut(&mut self, range: RangeInclusive<usize>) -> &mut Self::Output {
let (start, end) = compute_range_inclusive(*range.start(), *range.end(), self.stack.len());
let slice = &mut self.stack[start..end];
slice.reverse();
slice
}
}

impl Index<OpStackElement> for OpStack {
type Output = BFieldElement;

Expand All @@ -198,12 +253,69 @@ impl Index<OpStackElement> for OpStack {
}
}

impl Index<Range<OpStackElement>> for OpStack {
type Output = [BFieldElement];

fn index(&self, range: Range<OpStackElement>) -> &Self::Output {
let (start, end) = compute_range(
usize::from(range.start),
usize::from(range.end),
self.stack.len(),
);
let mut stack = self.stack[start..end].to_vec();
stack.reverse();
Box::leak(Box::new(stack))
}
}

impl Index<RangeInclusive<OpStackElement>> for OpStack {
type Output = [BFieldElement];

fn index(&self, range: RangeInclusive<OpStackElement>) -> &Self::Output {
let (start, end) = compute_range_inclusive(
usize::from(range.start()),
usize::from(range.end()),
self.stack.len(),
);
let mut stack = self.stack[start..end].to_vec();
stack.reverse();

Box::leak(Box::new(stack))
}
}

impl IndexMut<OpStackElement> for OpStack {
fn index_mut(&mut self, stack_element: OpStackElement) -> &mut Self::Output {
&mut self[usize::from(stack_element)]
}
}

impl IndexMut<Range<OpStackElement>> for OpStack {
fn index_mut(&mut self, range: Range<OpStackElement>) -> &mut Self::Output {
let (start, end) = compute_range(
usize::from(range.start),
usize::from(range.end),
self.stack.len(),
);
let slice = &mut self.stack[start..end];
slice.reverse();
slice
}
}

impl IndexMut<RangeInclusive<OpStackElement>> for OpStack {
fn index_mut(&mut self, range: RangeInclusive<OpStackElement>) -> &mut Self::Output {
let (start, end) = compute_range_inclusive(
usize::from(range.start()),
usize::from(range.end()),
self.stack.len(),
);
let slice = &mut self.stack[start..end];
slice.reverse();
slice
}
}

impl IntoIterator for OpStack {
type Item = BFieldElement;
type IntoIter = std::vec::IntoIter<Self::Item>;
Expand Down Expand Up @@ -1025,4 +1137,159 @@ mod tests {
let expected_len = 2 * OpStackElement::COUNT - 1;
prop_assert_eq!(expected_len, op_stack.len());
}

fn setup_op_stack() -> OpStack {
OpStack {
stack: bfe_vec![1, 2, 3, 4, 5],
underflow_io_sequence: vec![],
}
}

fn reversed_range(stack: &[BFieldElement], start: usize, end: usize) -> Vec<BFieldElement> {
stack[start..end].iter().rev().copied().collect()
}

#[test]
fn test_opstack_index_range() {
let op_stack = setup_op_stack();

test_opstack_range1_internal(&op_stack, 0..5, 0..5);
test_opstack_range1_internal(&op_stack, 0..2, 3..5);
test_opstack_range1_internal(&op_stack, 1..3, 2..4);
}

fn test_opstack_range1_internal(
op_stack: &OpStack,
actual_range: Range<usize>,
expected_range: Range<usize>,
) {
let (start, end) = (actual_range.start, actual_range.end);
let actual = op_stack[start..end].to_vec();

let (start, end) = (expected_range.start, expected_range.end);
let expected = reversed_range(&op_stack.stack, start, end);
assert_eq!(actual, expected);
}

// #[test]
// fn test_opstack_index_range1() {
// let op_stack = setup_op_stack();
// let len = op_stack.stack.len();

// // Test boundary ranges
// let actual = op_stack[0..len].to_vec();
// let expected = reversed_range(&op_stack.stack, 0, len);
// assert_eq!(actual, expected);

// // Test start boundary range
// let actual = op_stack[0..2].to_vec();
// let expected = reversed_range(&op_stack.stack, 3, len);
// assert_eq!(actual, expected);

// // Test typical range
// let actual = op_stack[1..3].to_vec();
// let expected = reversed_range(&op_stack.stack, 2, 4);
// assert_eq!(actual, expected);

// // ADDRESS len = 5, how to handle out of bounds in vector?
// // let actual = op_stack[1..6].to_vec();
// }

#[test]
fn test_opstack_index_range_inclusive() {
let op_stack = setup_op_stack();
let len = op_stack.stack.len();
let last = len - 1;

// Test boundary ranges
let actual = op_stack[0..=last].to_vec();
let expected = reversed_range(&op_stack.stack, 0, len);
assert_eq!(actual, expected);

// Test start boundary range
let actual = op_stack[0..=2].to_vec();
let expected = reversed_range(&op_stack.stack, 2, len);
assert_eq!(actual, expected);

// Test typical range
let actual = op_stack[1..=3].to_vec();
let expected = reversed_range(&op_stack.stack, 1, 4);
assert_eq!(actual, expected);
}

fn test_opstack_range_mut_inclusive(
actual_range: RangeInclusive<usize>,
expected_range: RangeInclusive<usize>,
) {
let mut op_stack = setup_op_stack();

let (start, end) = (*expected_range.start(), *expected_range.end());
let mut expected = op_stack.stack.clone()[start..=end].to_vec();
expected.reverse();

let (start, end) = (*actual_range.start(), *actual_range.end());
let actual = op_stack.index_mut(start..=end);

assert_eq!(actual, expected);
}

#[test]
fn test_opstack_index_mut_range_inclusive() {
test_opstack_range_mut_inclusive(0..=4, 0..=4);
test_opstack_range_mut_inclusive(0..=2, 2..=4);
test_opstack_range_mut_inclusive(1..=2, 2..=3);
}

// #[test]
// fn test_opstack_index_mut_range_inclusive() {
// let mut op_stack = setup_op_stack();
// let len = op_stack.stack.len();
// let mut expected = op_stack.stack.clone();
// expected.reverse();

// // Test boundary ranges
// let actual = op_stack.index_mut(0..=len-1);
// assert_eq!(actual, expected);

// // Test start boundary range
// let mut op_stack = setup_op_stack();
// let mut expected = op_stack.stack.clone()[2..].to_vec();
// expected.reverse();
// let actual = op_stack.index_mut(0..=2);
// assert_eq!(actual, expected);

// // Test typical range
// let mut op_stack = setup_op_stack();
// let mut expected = op_stack.stack.clone()[2..4].to_vec();
// expected.reverse();
// let actual = op_stack.index_mut(1..=2);
// assert_eq!(actual, expected);
// }

// #[test]
// fn test_opstack_index_mut_range_inclusive_usize() {
// let mut op_stack = setup_op_stack();
// let len = op_stack.stack.len();
// let last = len - 1;
// let mut expected = op_stack.stack.clone();
// expected.reverse();

// // Test boundary ranges
// let actual = op_stack.index_mut(0..=last);
// assert_eq!(actual, expected);

// // Test start boundary range
// let mut op_stack = setup_op_stack();
// let mut expected = op_stack.stack.clone()[2..].to_vec();
// expected.reverse();
// let actual = op_stack.index_mut(0..=2);
// assert_eq!(actual, expected);

// // Test typical range
// let mut op_stack = setup_op_stack();
// let mut expected = op_stack.stack.clone()[1..4].to_vec();
// expected.reverse();
// let actual = op_stack.index_mut(1..=3);
// assert_eq!(actual, expected);
// }
}