-
Notifications
You must be signed in to change notification settings - Fork 91
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.
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
Update toolchain #847
base: dev
Are you sure you want to change the base?
Update toolchain #847
Conversation
ca1692e
to
0fae216
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #847 +/- ##
==========================================
+ Coverage 91.90% 92.85% +0.94%
==========================================
Files 92 91 -1
Lines 12604 12166 -438
Branches 12604 12166 -438
==========================================
- Hits 11584 11297 -287
+ Misses 910 797 -113
+ Partials 110 72 -38 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
0fae216
to
6c25d63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 38 files at r1, 2 of 7 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 8 of 44 files reviewed, 2 unresolved discussions (waiting on @andrewmilson)
crates/prover/Cargo.toml
line 57 at r3 (raw file):
rust-2018-idioms = "deny" unused = "deny"
Rebase and remove:
[package.metadata.cargo-machete]
ignored = ["downcast-rs"]
crates/prover/src/core/backend/cpu/quotients.rs
line 80 at r3 (raw file):
/// Precomputes the complex conjugate line coefficients for each column in each sample batch. /// /// For the `i`-th (in a sample batch) column's numerator term `alpha^i * (c * F(p) - (a * p.y +
Why am I seeing changes in comments and other changes in the code? Is it somehow related to the toolchain update?
6c25d63
to
101a6c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 6 of 44 files reviewed, 2 unresolved discussions (waiting on @alonh5)
crates/prover/Cargo.toml
line 57 at r3 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Rebase and remove:
[package.metadata.cargo-machete] ignored = ["downcast-rs"]
Done.
crates/prover/src/core/backend/cpu/quotients.rs
line 80 at r3 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Why am I seeing changes in comments and other changes in the code? Is it somehow related to the toolchain update?
The new toolchain was throwing errors for comments.
Mainly complaining the summaries were too long and needing a empty line in places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 16 of 38 files at r1, 4 of 7 files at r2, 18 of 18 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @andrewmilson)
.github/workflows/ci.yaml
line 163 at r4 (raw file):
steps: - uses: actions/checkout@v4 <<<<<<< HEAD
Resolve.
Code quote:
<<<<<<< HEAD
crates/prover/src/lib.rs
line 1 at r4 (raw file):
#![allow(incomplete_features)]
Can you pleae explain the changes here?
crates/prover/src/core/backend/simd/m31.rs
line 288 at r4 (raw file):
} cfg_if::cfg_if! {
Is this also something that the new toolchain required?
crates/prover/src/core/backend/simd/lookups/gkr.rs
line 28 at r4 (raw file):
// Start DP with CPU backend to avoid dealing with instances smaller than a SIMD vector. let (y_rem, y_last_chunk) = y.split_last_chunk::<{ LOG_N_LANES as usize }>().unwrap();
How did you catch this? Did tests fail?
crates/prover/src/core/pcs/mod.rs
line 39 at r4 (raw file):
pow_bits: 5, // fri_config: FriConfig::new(0, 1, 3), fri_config: FriConfig::new(0, 1, 50),
On purpose?
Code quote:
// fri_config: FriConfig::new(0, 1, 3),
fri_config: FriConfig::new(0, 1, 50),
crates/prover/src/core/vcs/blake2s_ref.rs
line 34 at r4 (raw file):
#[inline(always)] fn rot16(x: u32) -> u32 { x.rotate_left(16)
Why is this left and all the others right? is it because for 16 it's identical?
101a6c4
to
52d050c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 41 of 44 files reviewed, 6 unresolved discussions (waiting on @alonh5)
.github/workflows/ci.yaml
line 163 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Resolve.
Done.
crates/prover/src/lib.rs
line 1 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Can you pleae explain the changes here?
stdarch_x86_avx512
replaces stdsimd
as the feature to use the avx512 intrinsics.
Added as cfg_attr
since stdarch_x86_avx512
feature only exists on x86
crates/prover/src/core/backend/simd/m31.rs
line 288 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Is this also something that the new toolchain required?
Not entirely but made sense to combine the change.
The error the toolchain had was about Swizzle, InterleaveEvens, InterleaveOdds being unused on non avx2/avx512
crates/prover/src/core/backend/simd/lookups/gkr.rs
line 28 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
How did you catch this? Did tests fail?
Yes exactly.
No compile issue so was a bit perplexed at the test failures initially.
crates/prover/src/core/pcs/mod.rs
line 39 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
On purpose?
My bad. Thanks
crates/prover/src/core/vcs/blake2s_ref.rs
line 34 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Why is this left and all the others right? is it because for 16 it's identical?
Oh funny, didn't notice this. Yeah I guess they are.
Rust analyser detected the bit shifts as a rotate and offered a quick fix which I applied.
Just changed it to rotate_right
to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)
crates/prover/src/core/backend/simd/lookups/gkr.rs
line 28 at r4 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…
Yes exactly.
No compile issue so was a bit perplexed at the test failures initially.
Weird they would change the order like that.
crates/prover/src/core/vcs/blake2s_ref.rs
line 34 at r4 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…
Oh funny, didn't notice this. Yeah I guess they are.
Rust analyser detected the bit shifts as a rotate and offered a quick fix which I applied.
Just changed it torotate_right
to be consistent.
Great thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this PR be closed?
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)
Hi, would the toolchain be updated later though? it is a bit hard to integrate stwo to other projects due to the toolchain "nightly-2024-01-04", would be nice to have a newer version. :) |
97084f0
to
44a5c93
Compare
44a5c93
to
e6d10bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2
.
Benchmark suite | Current: e6d10bc | Previous: f6214d1 | Ratio |
---|---|---|---|
SecureField add |
103999700 ns/iter (± 1387260 ) |
22737535 ns/iter (± 211082 ) |
4.57 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @shaharsamocha7
Hi! Nice this PR is opened again! Great job! Our repo now is using this branch (andrew/dev/update-toolchain) due to the old tool-chain of dev branch is not compatible with our project, so we really look forward for this PR to be merged. |
ef2e190
to
3baf7ae
Compare
3baf7ae
to
1f91c92
Compare
# Add Support for Constant Columns in Stwo Backend Stwo's recent development introduced new APIs to support constant/pre-processed columns. However, the `dev` branch of Stwo is still using an older nightly toolchain, which is incompatible with Powdr. Currently, Stwo has two open PRs [PR1](starkware-libs/stwo#847), [PR2](starkware-libs/stwo#871) aimed at updating the toolchain to nightly `11-06`. Previously, our Stwo backend dependency relied on one of these PRs' branches. However, this branch is no longer updated with the latest commits from their `dev` branch. These new commits are required to support constant columns. ### Temporary Solution To address this issue temporarily: - I moved Stwo's dependency to my fork of Stwo, where the branch is updated with both the latest `dev` branch commits and the newer toolchain. --- ### Tasks in this PR 1. **Add APIs to support constant columns**: 2. **Update test cases**: - Modify and add test cases to validate constant column support. --- --------- Co-authored-by: Thibaut Schaeffer <[email protected]>
This change is