Skip to content

Conversation

@nicoburns
Copy link
Contributor

Adds feature flags to vello_cpus u8 and f32 pipelines so that you can save on binary size if you're only planning on using one of them. Disabling the f32 pipeline knocks ~60kb off the binary size for me ~250kb -> ~190kb.

@nicoburns nicoburns requested a review from LaurenzV November 17, 2025 01:55
@nicoburns nicoburns added the C-cpu Applies to the vello_cpu crate label Nov 17, 2025
@LaurenzV
Copy link
Contributor

LaurenzV commented Nov 17, 2025

I’m not sure locking enum variants behind a feature flag is a good idea because it might break existing matching code downstream if someone activates one of the features.

@nicoburns
Copy link
Contributor Author

I’m not sure locking enum variants behind a feature flag is a good idea because it might break existing matching code downstream if someone activates one of the features.

Yeah, good point. We could mark the enum as non-exhaustive. But perhaps it's better to just keep both variants but ignore the value passed if only one pipeline is enabled? The enum values are "phrased" to be hint-like anyway...

@LaurenzV
Copy link
Contributor

Yes I think ignoring is better.

@nicoburns
Copy link
Contributor Author

@DJMcNab Any ideas on how to setup CI for this:

  • Two new features u8_pipeline and f32_pipeline.
  • Only exist for vello_cpu crate
  • Should ideally be a compile error if neither of them is specified

Perhaps we need to exclude vello_cpu from the workspace cargo-hack and test it separately?

@DJMcNab
Copy link
Member

DJMcNab commented Nov 17, 2025

I think it should be able to be treated in the same way that we treat std and libm.

@nicoburns nicoburns force-pushed the feature-flag-cpu-pipelines branch 8 times, most recently from 340e473 to 11d20b0 Compare December 6, 2025 15:50
@nicoburns
Copy link
Contributor Author

I have fixed CI by enabling both pipelines everywhere we're not explicitly checking that it compiles with just one of them enabled, and I've added a compile_error! for the case where neither pipeline is enabled.

@nicoburns nicoburns requested a review from DJMcNab December 6, 2025 15:51
@LaurenzV
Copy link
Contributor

LaurenzV commented Dec 7, 2025

It seems like there are still failures

@nicoburns nicoburns force-pushed the feature-flag-cpu-pipelines branch 4 times, most recently from 9f4e158 to a38b134 Compare December 7, 2025 16:25
@nicoburns
Copy link
Contributor Author

It seems like there are still failures

Ah, I think the compile_error! when neither feature is enabled reintroduced CI failures. Fixed.

@LaurenzV
Copy link
Contributor

LaurenzV commented Dec 7, 2025

I'm wondering whether it make sense to only enable the u8 pipeline by default. IMO, that's pretty much always the one you should use. But not sure.

@nicoburns nicoburns force-pushed the feature-flag-cpu-pipelines branch from a38b134 to 799a5fc Compare December 10, 2025 21:05
@nicoburns
Copy link
Contributor Author

I'm wondering whether it make sense to only enable the u8 pipeline by default. IMO, that's pretty much always the one you should use. But not sure.

I would agree with this. I've made the f32_pipeline feature non-default.

Copy link
Contributor

@LaurenzV LaurenzV left a comment

Choose a reason for hiding this comment

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

A bit annoying to make this work with CI, but it is what it is. 😄

@nicoburns
Copy link
Contributor Author

A bit annoying to make this work with CI, but it is what it is. 😄

There is an --at-least-one-of flag to cargo hack that looks like it would make it nicer. But that requires changing to --feature-powerset rather than using --each-feature which seems like it would be a much bigger change.

@nicoburns nicoburns force-pushed the feature-flag-cpu-pipelines branch from 67625cd to 8608763 Compare December 10, 2025 23:40
@nicoburns nicoburns force-pushed the feature-flag-cpu-pipelines branch from 8608763 to a7e172e Compare December 11, 2025 00:00
@nicoburns nicoburns added this pull request to the merge queue Dec 11, 2025
Merged via the queue into linebender:main with commit 0c242cb Dec 11, 2025
17 checks passed
@nicoburns nicoburns deleted the feature-flag-cpu-pipelines branch December 11, 2025 00:23
T-256 added a commit to T-256/vello that referenced this pull request Dec 11, 2025
github-merge-queue bot pushed a commit that referenced this pull request Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-cpu Applies to the vello_cpu crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants