-
Notifications
You must be signed in to change notification settings - Fork 287
Add the code for vello_api
#827
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
d780199
e494e87
b6d30c7
f451d82
f139609
ac870c2
d523737
91e0d02
e9e8dfc
6c2356c
ee4f55e
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 |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| // Copyright 2025 the Vello Authors | ||
| // SPDX-License-Identifier: Apache-2.0 OR MIT | ||
|
|
||
| //! Different execution modes for kernels. | ||
|
|
||
| #[derive(Copy, Clone, Debug)] | ||
| /// The execution mode used for the rendering process. | ||
| pub enum ExecutionMode { | ||
|
Member
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. This definitely needs to be |
||
| /// Only use scalar execution. This is recommended if you want to have | ||
| /// consistent results across different platforms and want to avoid unsafe code, | ||
| /// and is the only option if you disabled the `simd` feature. Performance will be | ||
| /// worse, though. | ||
| Scalar, | ||
| /// Select the best execution mode according to what is available on the host system. | ||
| /// This is the recommended option for highest performance. | ||
| #[cfg(feature = "simd")] | ||
|
Member
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. I don't think this should be feature flagged; we should just always have this be the default |
||
| Auto, | ||
|
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. Do I understand correctly that WebAssembly has its own wasm32-simd128 instruction set (currently in beta), and that this instruction set is not currently supported in the current implementation?
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. Yes, I currently did not write any SIMD code for that, but it's definitely planned. But it probably won't be a focus of my thesis (unless I implement everything else and still have time).
Member
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.
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. I don’t think it’s beta, at least tiny-skia uses WASM intrinsics and works fine on stable, AFAIK?
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. Oh, yeah, sorry! I shouldn’t have written “beta”. What I meant is that there’s still a large group of users on Safari < 16.4, which don’t support WASM SIMD. |
||
| /// Force the usage of neon SIMD instructions. This will lead to panics in case | ||
| /// the CPU doesn't support the target feature `neon`. | ||
| #[cfg(all(target_arch = "aarch64", feature = "simd"))] | ||
| Neon, | ||
| /// Force the usage of AVX2 SIMD instructions. This will lead to panics in case | ||
| /// the CPU doesn't support the target features `avx2` and `fma`. | ||
| #[cfg(all(target_arch = "x86_64", feature = "simd"))] | ||
| Avx2, | ||
| } | ||
|
Comment on lines
+6
to
+26
Member
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. One alternative would be to lift this to the type-level to allow for free branching due to monomorphization, but perhaps that should be an implementation detail and not be exposed on the API. On the other hand, if we ever get something like struct target features, the consumer could guide autovectorization through monomorphization. That said, I'd be happy to land this as-is and revisit when we have a clearer picture.
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. If I understand you correctly that's what already happens in my code (https://github.com/LaurenzV/cpu-sparse-experiments/blob/main/crates/sparse_primitives/src/lib.rs), the main reason for having it as an enum in addition is that you can select a mode at runtime, which is a nice feature to have IMO. But yes, I think those details can be discussed at a later point. :D |
||
|
|
||
| #[cfg(feature = "simd")] | ||
| impl Default for ExecutionMode { | ||
| fn default() -> Self { | ||
| Self::Auto | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(feature = "simd"))] | ||
| impl Default for ExecutionMode { | ||
| fn default() -> Self { | ||
| Self::Scalar | ||
| } | ||
| } | ||
|
Comment on lines
+28
to
+40
Member
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. I'm 90% sure that these could use the |
||
|
|
||
| /// Scalar execution mode. | ||
| #[derive(Debug)] | ||
| pub struct Scalar; | ||
|
|
||
| #[cfg(all(target_arch = "aarch64", feature = "simd"))] | ||
| #[derive(Debug)] | ||
| /// Execute using NEON intrinsics. | ||
| pub struct Neon; | ||
|
|
||
| #[cfg(all(target_arch = "x86_64", feature = "simd"))] | ||
| #[derive(Debug)] | ||
| /// Execute using AVX2 intrinsics. | ||
| pub struct Avx2; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| // Copyright 2025 the Vello Authors | ||
| // SPDX-License-Identifier: Apache-2.0 OR MIT | ||
|
|
||
| //! Types for paints. | ||
|
|
||
| use peniko::color::{AlphaColor, Srgb}; | ||
|
|
||
| // TODO: This will probably turn into a generic type where | ||
| // vello-hybrid and vello-cpu provide their own instantiations for | ||
| // a `Pattern` type. | ||
| /// A paint used for filling or stroking paths. | ||
| #[derive(Debug, Clone)] | ||
| pub enum Paint { | ||
|
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. Can this be used only internally, or is it also available for external consumers? My initial thought about I was imagining something similar here — where consumers of With that in mind, I think it’d be great to add an example showcasing how to use it later. Also, adding some common types and structures to That said, at this stage, Curious to hear your thoughts — maybe we can chat about this during Office Hours?
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.
Would this be a runtime switch or build time feature? Runtime determination could see increased bundle sizes
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. For the web target, I believe it would be better to introduce a build-time feature, such as cpu, hybrid, etc., to prevent unnecessary increases in bundle size. This is something we’ll need to work out.
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.
I think that will already be possible with the
Yeah as I mentioned, to me it's still not 100% clear what the purpose of
So, for
Member
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.
By lifting the CPU feature sets to the type-level the compiler is able to remove unused specializations after monomorphization, also for explicit branches like checking for
Member
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. I'm not actually sure what this merged content meaningfully represents as a grouping. Almost all of this should probably be in Vello Common? Certainly the SIMD stuff seems a bit out-of-place in this crate.
Member
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. There seemingly were some different ideas on what exactly
Member
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. In my mind, the crate which does the dynamic dispatch would be Vello, probably.
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.
How would that work if you use
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. I agree with @DJMcNab on deferring the use of
It looks like there are different understandings of that crate. How about we discuss it later — either in Zulip or during Office Hours? For now, what do you think about using |
||
| /// A solid color. | ||
| Solid(AlphaColor<Srgb>), | ||
| /// A gradient. | ||
| Gradient(()), | ||
| /// A pattern. | ||
| Pattern(()), | ||
| } | ||
|
|
||
| impl From<AlphaColor<Srgb>> for Paint { | ||
| fn from(value: AlphaColor<Srgb>) -> Self { | ||
| Self::Solid(value) | ||
| } | ||
| } | ||
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.
[nit]: Maybe I’m wrong, but I see this change as a sign that
vello_apiis now considered stable or at least working well enough to be fully integrated into the workspace, rather than remaining a separate experimental component. It suggests thatvello_apiis now a fundamental part of the project and can be used consistently across different crates.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.
Hmm, I wouldn't see it this way, the only reason I added it here is so that everywhere else we use it we can just do
vello_api = {workspace = true}to avoid duplicating the path to the crate. I think everyone who works with this repo is aware that this is in-progress and experimental, so I wouldn't worry about it too much. Especially since we aren't gonna publish it.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 don't think adding a crate to the workspace signals readiness, but an argument could be made for ordering the sparse strip crates after the main Vello crates. A note could then be added stating they're the experimental sparse strip crates.