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.

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

spirv-std: only enable glam&num-traits's libm features on SPIR-V targets. #1122

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Jan 28, 2024

Draft because I'm not sure if we should do some kind of automated testing for this, I can't think of an obvious way to know "slower floating-point algorithms are being used" (also there's #1115 which may conflict).

Testing-wise the best we can do is probably likely this:

$ cargo tree -e features -p spirv-std | rg libm
├── glam feature "libm"
│       └── libm v0.2.8
├── num-traits feature "libm"
│       └── libm feature "default"
│           └── libm v0.2.8

(the above is before this PR, and there are no libm results after this PR)

@eddyb eddyb requested a review from fornwall January 28, 2024 02:47
@fornwall fornwall marked this pull request as ready for review January 31, 2024 13:42
@fornwall fornwall enabled auto-merge (squash) February 9, 2024 07:51
@fornwall fornwall enabled auto-merge (squash) February 26, 2024 11:03
@fornwall fornwall merged commit 9d80951 into EmbarkStudios:main Feb 26, 2024
7 checks passed
@eddyb eddyb deleted the libmno branch February 27, 2024 07:19
@eddyb
Copy link
Contributor Author

eddyb commented Feb 27, 2024

Accidentally found that this is really broken without resolver = "2"/edition = "2021".
The ability itself to have per-target feature controls was added then (I now remember struggling in 2020 etc.).

Not sure how to control the resolver from spirv-builder but I will look around for a solution.

EDIT: one very important data point is that this problem is inevitable - you can't really have target-controlled dependencies before resolver = "2" without constant massive issues, and spirv-std may eventually want to e.g. emulate image sampling when used on CPU. (this is something that has been requested for over a year but I didn't manage to get back to. it would also massively help with testing, and outside of weird anisotropic stuff, the sampling math is defined by Vulkan and it should be possible to check GPU against CPU)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

adding spirv-std anywhere in the worskspace, slows down the CPU code due to libm feature
2 participants