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

Jump to bottom

feat: add iterator-based for loops #1176

Merged
merged 42 commits into from
Jan 12, 2025
Merged

feat: add iterator-based for loops #1176

merged 42 commits into from
Jan 12, 2025

Conversation

yi-sun
Copy link
Collaborator

@yi-sun yi-sun commented Jan 6, 2025 •
edited
Loading

Closes INT-2930, INT-2946

@yi-sun yi-sun added run-benchmark triggers benchmark workflows on the pr run-benchmark-e2e labels Jan 6, 2025
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@yi-sun yi-sun force-pushed the feat/optimize-for-loop branch from a8c0bcc to 165c11c Compare January 7, 2025 00:39
@yi-sun yi-sun force-pushed the feat/optimize-for-loop branch from 6d8afc2 to f69afb5 Compare January 7, 2025 04:05
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@yi-sun yi-sun force-pushed the feat/optimize-for-loop branch from 1f9f911 to b242cd7 Compare January 7, 2025 05:35
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@yi-sun yi-sun force-pushed the feat/optimize-for-loop branch from b242cd7 to e6b8852 Compare January 8, 2025 05:37
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@yi-sun yi-sun marked this pull request as ready for review January 8, 2025 08:42
@yi-sun yi-sun changed the title wip: optimize for loops feat: add iterator-based for loops Jan 8, 2025
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@yi-sun yi-sun force-pushed the feat/optimize-for-loop branch from 24c5898 to f92aea1 Compare January 9, 2025 04:24
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@yi-sun yi-sun force-pushed the feat/optimize-for-loop branch from 62c5f64 to 7d8670b Compare January 10, 2025 07:32
yi-sun
yi-sun commented Jan 10, 2025
View reviewed changes
extensions/native/compiler/src/ir/collections.rs
@@ -78,10 +78,9 @@ impl<C: Config, V: MemVariable<C>> Array<C, V> {
}
}
Self::Dyn(ptr, len) => {
assert_eq!(V::size_of(), 1, "only support variables of size 1");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure why this restriction was previously present, as it seems easily removed on L83 below.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
zlangley
zlangley approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@zlangley zlangley left a comment

Choose a reason for hiding this comment

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

lgtm

extensions/native/compiler/src/ir/utils.rs Outdated Show resolved Hide resolved
extensions/native/compiler/src/asm/compiler.rs Show resolved Hide resolved
extensions/native/compiler/src/ir/builder.rs Outdated Show resolved Hide resolved
extensions/native/compiler/src/ir/collections.rs Outdated Show resolved Hide resolved
extensions/native/compiler/src/ir/collections.rs Show resolved Hide resolved
@yi-sun yi-sun requested a review from zlangley January 10, 2025 20:24
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
zlangley
zlangley reviewed Jan 10, 2025
View reviewed changes
extensions/native/compiler/src/asm/compiler.rs Outdated Show resolved Hide resolved
extensions/native/compiler/src/asm/compiler.rs
debug_info.clone(),
);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Small optimization: We can put the Bne after the first AddFI so that we don't spend cycles incrementing pointers we don't need to use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need the first Bne to cover the case where starts[0] == end0.

extensions/native/compiler/src/ir/builder.rs Outdated Show resolved Hide resolved
extensions/native/compiler/src/ir/builder.rs Show resolved Hide resolved
extensions/native/compiler/src/ir/collections.rs Show resolved Hide resolved
extensions/native/compiler/src/ir/collections.rs Show resolved Hide resolved
extensions/native/recursion/src/fri/mod.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@yi-sun yi-sun requested a review from zlangley January 11, 2025 00:15
@github-actions GitHub Actions
Copy link

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (-165 [-4.3%]) 3,639 (+1104 [+0.1%]) 747,676 (-2671369 [-8.9%]) 27,344,405 - - -
fibonacci_program 6,100 1,500,137 51,505,102 (-574 [-3.8%]) 14,379 (+22948 [+0.7%]) 3,193,718 (-10251818 [-8.0%]) 118,598,779
regex_program 18,467 4,190,904 165,028,173 (-425 [-1.4%]) 29,940 (-427370 [-6.6%]) 6,096,409 (-33027600 [-11.3%]) 258,281,989
fib_e2e 42,650 12,000,137 410,820,430 87,334 19,128,868 707,488,130
ecrecover_program (+3 [+0.1%]) 2,619 285,401 15,092,297 (-2962 [-6.9%]) 39,660 (-793509 [-8.2%]) 8,869,846 (-53337248 [-12.1%]) 387,220,670

Commit: 4a1d614

Benchmark Workflow

@yi-sun yi-sun merged commit fc0be52 into native-cleanup Jan 12, 2025
19 checks passed
@yi-sun yi-sun deleted the feat/optimize-for-loop branch January 12, 2025 05:51
yi-sun added a commit that referenced this pull request Jan 12, 2025
@yi-sun
feat: add iterator-based for loops (#1176)
8a13b3a 
* chore: clean up comments in native compiler

* feat: add iterator for loop

* feat: convert more for loops

* fix: lint

* wip: zipped iterator

* fix: lint

* feat: iterator for loop in poseidon

* feat: optimize store

* fix: keep loop variable

* fix: store

* opt: try removing get_ref

* opt: remove duplicate get

* opt: remove idx

* chore: remove unused code

* fix: lint

* feat: add ZipFor and zipped_iter

* fix: lint

* feat: implement zipped_iter

* fix: faulty assert

* fix: read

* fix: lint

* fix: lint

* fix: vec

* feat: add more zipped_iter

* feat: optimize verify_batch

* fix: use first array for zipped_iter

* feat: add zipped_iter to hint

* chore: remove print

* feat: optimize hint_words

* fix: lifetime issues

* chore: add comment with example

* feat: use square-and-multiply for exp_reverse_bits_len

* fix: input size for exp_reverse_bits_len

* fix: slice and shift for V::size_of() greater than 1, optimization

* fix: replace incorrectly removed side effect

* feat: optimize adds

* feat: optimize for loops in duplex challenger

* chore: use larger runner for build workflow

* feat: address comments

* feat: remove all but the first end ptr in ZipFor

* feat: implement and use compile_zip! macro

* feat: add tests for iter and zip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyunyunyunyu nyunyunyunyu Awaiting requested review from nyunyunyunyu

@TlatoaniHJ TlatoaniHJ Awaiting requested review from TlatoaniHJ

@zlangley zlangley Awaiting requested review from zlangley

Assignees
No one assigned
Labels
run-benchmark triggers benchmark workflows on the pr run-benchmark-e2e
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yi-sun @zlangley