Skip to content

Commit

Permalink
refactor!: Remove field from some “list” snippets
Browse files Browse the repository at this point in the history
In particular, remove the field `element_type` from the structs
- `list::new::New`,
- `list::length::Length`, and
- `list::set_length::SetLength`.

The element type does not influence the snippets' behavior.
  • Loading branch information
jan-ferdinand committed Jan 21, 2025
1 parent e6a4436 commit ca6da23
Show file tree
Hide file tree
Showing 27 changed files with 97 additions and 228 deletions.
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[
{
"name": "tasmlib_list_length___digest",
"name": "tasmlib_list_length",
"benchmark_result": {
"clock_cycle_count": 5,
"hash_table_height": 6,
Expand All @@ -11,7 +11,7 @@
"case": "CommonCase"
},
{
"name": "tasmlib_list_length___digest",
"name": "tasmlib_list_length",
"benchmark_result": {
"clock_cycle_count": 5,
"hash_table_height": 6,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[
{
"name": "tasmlib_list_new___digest",
"name": "tasmlib_list_new",
"benchmark_result": {
"clock_cycle_count": 32,
"hash_table_height": 36,
Expand All @@ -11,7 +11,7 @@
"case": "CommonCase"
},
{
"name": "tasmlib_list_new___digest",
"name": "tasmlib_list_new",
"benchmark_result": {
"clock_cycle_count": 32,
"hash_table_height": 36,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[
{
"name": "tasmlib_list_set_length___digest",
"name": "tasmlib_list_set_length",
"benchmark_result": {
"clock_cycle_count": 7,
"hash_table_height": 12,
Expand All @@ -11,7 +11,7 @@
"case": "CommonCase"
},
{
"name": "tasmlib_list_set_length___digest",
"name": "tasmlib_list_set_length",
"benchmark_result": {
"clock_cycle_count": 7,
"hash_table_height": 12,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"name": "tasmlib_neptune_mutator_get_swbf_indices_1048576_45",
"benchmark_result": {
"clock_cycle_count": 4395,
"hash_table_height": 481,
"hash_table_height": 475,
"u32_table_height": 4633,
"op_stack_table_height": 3184,
"ram_table_height": 470
Expand All @@ -14,7 +14,7 @@
"name": "tasmlib_neptune_mutator_get_swbf_indices_1048576_45",
"benchmark_result": {
"clock_cycle_count": 4395,
"hash_table_height": 481,
"hash_table_height": 475,
"u32_table_height": 4507,
"op_stack_table_height": 3184,
"ram_table_height": 470
Expand Down
4 changes: 2 additions & 2 deletions tasm-lib/benchmarks/tasmlib_verifier_fri_verify.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"name": "tasmlib_verifier_fri_verify",
"benchmark_result": {
"clock_cycle_count": 43998,
"hash_table_height": 14682,
"hash_table_height": 14652,
"u32_table_height": 17999,
"op_stack_table_height": 42235,
"ram_table_height": 17563
Expand All @@ -14,7 +14,7 @@
"name": "tasmlib_verifier_fri_verify",
"benchmark_result": {
"clock_cycle_count": 43998,
"hash_table_height": 14682,
"hash_table_height": 14652,
"u32_table_height": 17395,
"op_stack_table_height": 42235,
"ram_table_height": 17563
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"name": "tasmlib_verifier_stark_verify_dynamic_inner_padded_height_256_fri_exp_4",
"benchmark_result": {
"clock_cycle_count": 188148,
"hash_table_height": 138847,
"hash_table_height": 138823,
"u32_table_height": 25078,
"op_stack_table_height": 167240,
"ram_table_height": 289169
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"name": "tasmlib_verifier_stark_verify_dynamic_inner_padded_height_512_fri_exp_4",
"benchmark_result": {
"clock_cycle_count": 195637,
"hash_table_height": 146539,
"hash_table_height": 146515,
"u32_table_height": 33679,
"op_stack_table_height": 171942,
"ram_table_height": 290320
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"name": "tasmlib_verifier_stark_verify_static_inner_padded_height_256_fri_exp_4",
"benchmark_result": {
"clock_cycle_count": 176445,
"hash_table_height": 124807,
"hash_table_height": 124777,
"u32_table_height": 25078,
"op_stack_table_height": 159434,
"ram_table_height": 285270
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"name": "tasmlib_verifier_stark_verify_static_inner_padded_height_512_fri_exp_4",
"benchmark_result": {
"clock_cycle_count": 183934,
"hash_table_height": 132499,
"hash_table_height": 132469,
"u32_table_height": 33679,
"op_stack_table_height": 164136,
"ram_table_height": 286421
Expand Down
37 changes: 3 additions & 34 deletions tasm-lib/src/exported_snippets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -373,40 +373,9 @@ pub fn name_to_snippet(fn_name: &str) -> Option<Box<dyn BasicSnippet>> {
Some(Box::new(list::set::Set::new(DataType::Digest)))
}

"tasmlib_list_new___bool" => Some(Box::new(list::new::New::new(DataType::Bool))),
"tasmlib_list_new___u32" => Some(Box::new(list::new::New::new(DataType::U32))),
"tasmlib_list_new___u64" => Some(Box::new(list::new::New::new(DataType::U64))),
"tasmlib_list_new___bfe" => Some(Box::new(list::new::New::new(DataType::Bfe))),
"tasmlib_list_new___xfe" => Some(Box::new(list::new::New::new(DataType::Xfe))),
"tasmlib_list_new___digest" => Some(Box::new(list::new::New::new(DataType::Digest))),

"tasmlib_list_length___bool" => Some(Box::new(list::length::Length::new(DataType::Bool))),
"tasmlib_list_length___u32" => Some(Box::new(list::length::Length::new(DataType::U32))),
"tasmlib_list_length___u64" => Some(Box::new(list::length::Length::new(DataType::U64))),
"tasmlib_list_length___bfe" => Some(Box::new(list::length::Length::new(DataType::Bfe))),
"tasmlib_list_length___xfe" => Some(Box::new(list::length::Length::new(DataType::Xfe))),
"tasmlib_list_length___digest" => {
Some(Box::new(list::length::Length::new(DataType::Digest)))
}

"tasmlib_list_set_length___bool" => {
Some(Box::new(list::set_length::SetLength::new(DataType::Bool)))
}
"tasmlib_list_set_length___u32" => {
Some(Box::new(list::set_length::SetLength::new(DataType::U32)))
}
"tasmlib_list_set_length___u64" => {
Some(Box::new(list::set_length::SetLength::new(DataType::U64)))
}
"tasmlib_list_set_length___bfe" => {
Some(Box::new(list::set_length::SetLength::new(DataType::Bfe)))
}
"tasmlib_list_set_length___xfe" => {
Some(Box::new(list::set_length::SetLength::new(DataType::Xfe)))
}
"tasmlib_list_set_length___digest" => {
Some(Box::new(list::set_length::SetLength::new(DataType::Digest)))
}
"tasmlib_list_new" => Some(Box::new(list::new::New)),
"tasmlib_list_length" => Some(Box::new(list::length::Length)),
"tasmlib_list_set_length" => Some(Box::new(list::set_length::SetLength)),

"tasmlib_list_multiset_equality_digests" => Some(Box::new(
list::multiset_equality_digests::MultisetEqualityDigests,
Expand Down
4 changes: 2 additions & 2 deletions tasm-lib/src/hashing/algebraic_hasher/sample_indices.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ impl BasicSnippet for SampleIndices {
let then_reduce_and_save = format!("{entrypoint}_then_reduce_and_save");
let else_drop_tip = format!("{entrypoint}_else_drop_tip");

let new_list = library.import(Box::new(New::new(DataType::U32)));
let length = library.import(Box::new(Length::new(DataType::U32)));
let new_list = library.import(Box::new(New));
let length = library.import(Box::new(Length));
let push_element = library.import(Box::new(Push::new(DataType::U32)));

let if_can_sample = triton_asm! (
Expand Down
6 changes: 3 additions & 3 deletions tasm-lib/src/hashing/algebraic_hasher/sample_scalars.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@ impl BasicSnippet for SampleScalars {
assert_eq!(3, EXTENSION_DEGREE, "Code assumes extension degree 3");

let entrypoint = self.entrypoint();
let set_length = library.import(Box::new(SetLength::new(DataType::Xfe)));
let new_list_of_xfes = library.import(Box::new(New::new(DataType::Xfe)));
let set_length = library.import(Box::new(SetLength));
let new_list = library.import(Box::new(New));
let safety_offset = 1;
let squeeze_repeatedly = library.import(Box::new(SqueezeRepeatedly));
triton_asm! {
// BEFORE: _ num_scalars
// AFTER: _ *scalars
{entrypoint}:
call {new_list_of_xfes}
call {new_list}
// _ num_scalars *scalars

// set length
Expand Down
7 changes: 3 additions & 4 deletions tasm-lib/src/list/contiguous_list/get_pointer_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,10 @@ impl DeprecatedSnippet for GetPointerList {
fn function_code(&self, library: &mut Library) -> String {
let entrypoint = self.entrypoint_name();

let data_type = DataType::VoidPointer;
let get_list_length = library.import(Box::new(GetLength));
let new_list = library.import(Box::new(New::new(data_type.clone())));
let set_length = library.import(Box::new(SetLength::new(data_type.clone())));
let set_element = library.import(Box::new(Set::new(data_type)));
let new_list = library.import(Box::new(New));
let set_length = library.import(Box::new(SetLength));
let set_element = library.import(Box::new(Set::new(DataType::VoidPointer)));

format!(
"
Expand Down
2 changes: 1 addition & 1 deletion tasm-lib/src/list/higher_order/all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ impl BasicSnippet for All {
let output_type = self.f.range();
assert_eq!(output_type, DataType::Bool);

let get_length = library.import(Box::new(Length::new(input_type.clone())));
let get_length = library.import(Box::new(Length));
let list_get = library.import(Box::new(Get::new(input_type)));

let inner_function_name = match &self.f {
Expand Down
13 changes: 6 additions & 7 deletions tasm-lib/src/list/higher_order/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ impl BasicSnippet for Filter {
assert_eq!(output_type, DataType::Bool);

let safety_offset = LIST_METADATA_SIZE;
let get_length = library.import(Box::new(Length::new(input_type.clone())));
let get_length = library.import(Box::new(Length));
let list_get = library.import(Box::new(Get::new(input_type)));
let new_list = library.import(Box::new(New::new(output_type.clone())));
let set_length = library.import(Box::new(SetLength::new(output_type)));
let new_list = library.import(Box::new(New));
let set_length = library.import(Box::new(SetLength));
let element_size = self.f.domain().stack_size();

let inner_function_name = match &self.f {
Expand Down Expand Up @@ -176,7 +176,6 @@ mod tests {
memory: &mut HashMap<BFieldElement, BFieldElement>,
) {
let input_type = self.f.domain();
let output_type = self.f.range();

let element_size = self.f.domain().stack_size();
let memcpy = MemCpy::rust_shadowing;
Expand All @@ -189,13 +188,13 @@ mod tests {

let get_element = rust_shadowing_helper_functions::list::list_get;

New::new(input_type.clone()).rust_shadow(stack, memory);
New.rust_shadow(stack, memory);
let output_list = stack.pop().unwrap();

// set length
stack.push(output_list);
stack.push(BFieldElement::new(len as u64));
SetLength::new(output_type).rust_shadowing(stack, vec![], vec![], memory);
SetLength.rust_shadowing(stack, vec![], vec![], memory);
stack.pop();

// forall elements, read + map + maybe copy
Expand Down Expand Up @@ -236,7 +235,7 @@ mod tests {
// set length
stack.push(output_list);
stack.push(BFieldElement::new(output_index as u64));
SetLength::new(input_type).rust_shadowing(stack, vec![], vec![], memory);
SetLength.rust_shadowing(stack, vec![], vec![], memory);
}

fn pseudorandom_initial_state(
Expand Down
6 changes: 3 additions & 3 deletions tasm-lib/src/list/higher_order/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ impl<const NUM_INPUT_LISTS: usize> ChainMap<NUM_INPUT_LISTS> {
let output_type = self.f.range();
assert!(input_type.static_length().is_some());

let new_list = library.import(Box::new(New::new(output_type.clone())));
let new_list = library.import(Box::new(New));
let inner_fn = self.decompose_inner_fn(library);

let entrypoint = self.entrypoint();
Expand Down Expand Up @@ -266,7 +266,7 @@ impl<const NUM_INPUT_LISTS: usize> ChainMap<NUM_INPUT_LISTS> {
let output_type = self.f.range();
assert!(input_type.static_length().is_none());

let new_list = library.import(Box::new(New::new(output_type.clone())));
let new_list = library.import(Box::new(New));
let push = library.import(Box::new(Push::new(output_type.clone())));
let inner_fn = self.decompose_inner_fn(library);

Expand Down Expand Up @@ -482,7 +482,7 @@ mod tests {
let input_type = self.f.domain();
let output_type = self.f.range();

New::new(output_type.clone()).rust_shadow(stack, memory);
New.rust_shadow(stack, memory);
let output_list_pointer = stack.pop().unwrap();

let input_list_pointers = (0..NUM_INPUT_LISTS)
Expand Down
2 changes: 1 addition & 1 deletion tasm-lib/src/list/higher_order/zip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl BasicSnippet for Zip {
fn code(&self, library: &mut Library) -> Vec<LabelledInstruction> {
let output_type = DataType::Tuple(vec![self.left_type.clone(), self.right_type.clone()]);

let new_output_list = library.import(Box::new(New::new(output_type.clone())));
let new_output_list = library.import(Box::new(New));

let entrypoint = self.entrypoint();
let main_loop_label = format!("{entrypoint}_loop");
Expand Down
6 changes: 2 additions & 4 deletions tasm-lib/src/list/horner_evaluation_dynamic_length.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,14 @@ impl BasicSnippet for HornerEvaluationDynamicLength {
let entrypoint = self.entrypoint();
let loop_batches = format!("{entrypoint}_loop_batches");
let loop_remainder = format!("{entrypoint}_loop_remainder");
let length_of_list_of_xfes = library.import(Box::new(Length {
element_type: DataType::Xfe,
}));
let length_of_list = library.import(Box::new(Length));

triton_asm! {
// BEFORE: *coefficients [x]
// AFTER: [poly(x)]
{entrypoint}:
dup 3 // _ *coefficients [x] *coefficients
call {length_of_list_of_xfes} // _ *coefficients [x] num_coefficients
call {length_of_list} // _ *coefficients [x] num_coefficients
push 3 mul // _ *coefficients [x] size
dup 4 add // _ *coefficients [x] *last_coefficient+2
dup 3 dup 3 dup 3 // _ *coefficients [x] *last_coefficient+2 [x]
Expand Down
26 changes: 7 additions & 19 deletions tasm-lib/src/list/length.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,20 @@ use crate::traits::basic_snippet::SignOffFingerprint;
// Todo: the return type used to be a u32. However, neither the tasm nor the
// rust shadowing ever performed any range checks. Should the return type be
// changed back to u32? If so, should range checks be introduced?
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct Length {
pub element_type: DataType,
}

impl Length {
pub fn new(element_type: DataType) -> Self {
Self { element_type }
}
}
#[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Hash)]
pub struct Length;

impl BasicSnippet for Length {
fn inputs(&self) -> Vec<(DataType, String)> {
let list_type = DataType::List(Box::new(self.element_type.clone()));
vec![(list_type, "*list".to_string())]
vec![(DataType::VoidPointer, "*list".to_string())]
}

fn outputs(&self) -> Vec<(DataType, String)> {
vec![(DataType::Bfe, "list_length".to_string())]
}

fn entrypoint(&self) -> String {
let element_type = self.element_type.label_friendly_name();
format!("tasmlib_list_length___{element_type}")
"tasmlib_list_length".to_string()
}

fn code(&self, _: &mut Library) -> Vec<LabelledInstruction> {
Expand Down Expand Up @@ -97,7 +87,7 @@ mod tests {
let list_len = rng.gen_range(0..=1 << 10);

let mut memory = HashMap::default();
insert_random_list(&self.element_type, list_ptr, list_len, &mut memory);
insert_random_list(&DataType::Digest, list_ptr, list_len, &mut memory);
let stack = [self.init_stack_for_isolated_run(), vec![list_ptr]].concat();

AccessorInitialState { stack, memory }
Expand All @@ -106,9 +96,7 @@ mod tests {

#[test]
fn rust_shadow() {
ShadowedAccessor::new(Length::new(DataType::Bfe)).test();
ShadowedAccessor::new(Length::new(DataType::U64)).test();
ShadowedAccessor::new(Length::new(DataType::Digest)).test();
ShadowedAccessor::new(Length).test();
}
}

Expand All @@ -119,6 +107,6 @@ mod benches {

#[test]
fn length_long_benchmark() {
ShadowedAccessor::new(Length::new(DataType::Digest)).bench();
ShadowedAccessor::new(Length).bench();
}
}
2 changes: 1 addition & 1 deletion tasm-lib/src/list/multiset_equality_digests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl BasicSnippet for MultisetEqualityDigests {

fn code(&self, library: &mut Library) -> Vec<LabelledInstruction> {
let entrypoint = self.entrypoint();
let length_snippet = library.import(Box::new(Length::new(DataType::Digest)));
let length_snippet = library.import(Box::new(Length));
let hash_varlen = library.import(Box::new(HashVarlen));

let early_abort_label = format!("{entrypoint}_early_abort");
Expand Down
Loading

3 comments on commit ca6da23

@Sword-Smith
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you could argue that the set_length would need to know the element size to know if the new length would make the list too long? Such that it points to values outside of its memory region. It should be fine to only have this check in pop, push, set, and get, though. Agreed?

@jan-ferdinand
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my thinking was that set_length falls in the unsafe realms of list manipulation anyway, so if you use it, you better know what you're doing. It's probably a good idea to document this on the snippet. I plan on adding documentation to most of those snippets as I transition them away from DeprecatedSnippet.

@jan-ferdinand
Copy link
Member Author

Choose a reason for hiding this comment

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

For what it's worth, I also plan to add bounds checks to the indices passed to get and set. get currently has a check that the element-to-get fully resides within one memory page from the list pointer1, which is also a good addition for set, and potentially for push.

Footnotes

  1. there's no check regarding the alignment to memory pages; I'm not sure such a check would be beneficial, since lists are allowed to reside in non-deterministically initialized memory, where they do not have to be aligned to memory page boundaries

Please sign in to comment.