-
Notifications
You must be signed in to change notification settings - Fork 303
sparse_strips: Bump fearless_simd
#1853
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,7 @@ version = "0.10.0" | |
| edition = "2024" | ||
| # Keep in sync with RUST_MIN_VER in .github/workflows/ci.yml, with the relevant README.md files | ||
| # and with the MSRV in the `Unreleased` section of CHANGELOG.md. | ||
| rust-version = "1.88" | ||
| rust-version = "1.89" | ||
| license = "Apache-2.0 OR MIT" | ||
| repository = "https://github.com/linebender/vello" | ||
|
|
||
|
|
@@ -118,7 +118,7 @@ rayon = { version = "1.12.0" } | |
| thread_local = "1.1.9" | ||
| crossbeam-channel = "0.5.15" | ||
| ordered-channel = { version = "1.2.0", features = ["crossbeam-channel"] } | ||
| fearless_simd = { version = "0.4.0", default-features = false } | ||
| fearless_simd = { version = "0.7.0", default-features = false } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add browser wasm support to the benchmark harness? The web is an important target for us, and this would give us much better insight into its performance; once supported, it would also be useful to rerun the benchmarks for this fearless_simd bump to identify any improvements/regressions. |
||
|
|
||
| # Unlike the other crates, please do not update these before a release | ||
| # unless absolutely necessary. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -478,7 +478,7 @@ impl Tiles { | |
| pub fn sort_tiles(&mut self) { | ||
| self.sorted = true; | ||
| // To enable auto-vectorization. | ||
| self.level.dispatch(|_| self.tile_buf.sort_unstable()); | ||
| dispatch!(self.level, _ => self.tile_buf.sort_unstable()); | ||
| } | ||
|
|
||
| /// Get the tile at a certain index. | ||
|
|
@@ -636,11 +636,8 @@ impl Tiles { | |
| let current = f32x4::from_slice(s, target_row); | ||
|
|
||
| // See comment below on the double counting risk! | ||
| let double_count = if at_top_of_tile { | ||
| f_dir_v | ||
| } else { | ||
| f32x4::splat(s, 0.0) | ||
| }; | ||
| let double_count = | ||
| mask32x4::splat(s, at_top_of_tile).select(f_dir_v, f32x4::splat(s, 0.0)); | ||
| let next = fractional_coverage.mul_add(f_dir_v, current - double_count); | ||
| next.store_slice(target_row); | ||
| } | ||
|
|
@@ -789,11 +786,8 @@ impl Tiles { | |
| let target_row = &mut self.windings.partial[y_idx as usize]; | ||
| let current = f32x4::from_slice(s, target_row); | ||
|
|
||
| let double_count = if crosses_top { | ||
| f_dir_v | ||
| } else { | ||
| f32x4::splat(s, 0.0) | ||
| }; | ||
| let double_count = mask32x4::splat(s, crosses_top) | ||
| .select(f_dir_v, f32x4::splat(s, 0.0)); | ||
|
Comment on lines
639
to
+790
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On AVX2 wit the old code I got a regression of around 25% with new fearless_simd, because the branchless selects were turned into branches. These two changes fix this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can also use std's
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I think I'll keep it for now since I've already benchmarked this, but thanks for pointing it out! |
||
| let next = fractional_coverage | ||
| .mul_add(f_dir_v, current - double_count); | ||
| next.store_slice(target_row); | ||
|
|
||
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.
I’m concerned this adds more CI time by rerunning nearly the entire test suite for relatively uncommon AVX-512 hardware. Could we keep SSE2 coverage but run AVX-512 only when SIMD-related code changes, or in a nightly/weekly scheduled workflow instead of on every PR?
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.
This is not nearly as bad as it appears. CI time for this job is under 3 minutes; it does not make CI take longer overall.
And it's not as uncommon as you might expect. Steam hardware survey puts AVX-512 availability at 23%, which is far ahead of both SSE4.2-only (2%) and SSE2-only (1%) systems in their data.
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.
I would agree with @Shnatsel here, it definitely doesn't impact CI run times since it's not even close to being the bottleneck, and AVX-512 is not that uncommon anymore, especially on server hardware, which is also relevant for our internal use cases. The only downside is that it makes the CI file uglier. So unless you are strongly opposed, I think it would be better to keep this.
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.
Beyond runner time, this brings the workflow to 21 jobs while Linebender’s Free plan allows 20 concurrent GitHub-hosted jobs. The AVX-512 job took ~4 minutes and finished well before the slowest job, so it likely won’t materially increase total CI duration, but it can still occupy a scarce slot and cause queueing when the organization is busy.
Anyway, I’m not blocking this — just expressing concern about the gradual increase in CI infra usage, which I think we’ll need to address in the near future.
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.
Just to clarify, what do you mean by “SSE4.2-only (2%) and SSE2-only (1%)” here? The Steam survey seems to report support for each instruction set independently rather than mutually exclusive “only” buckets.
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.
I subtracted the AVX2 percentage from the SSE4.2 to get "SSE4.2 only". Same for "SSE2 only": SSE2 - SSE4.2.
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.
Interesting, didn't know about the 20 concurrent jobs limit, thanks for pointing it out!