Skip to content

Conversation

@Firestar99
Copy link
Member

Adds a new trait Derivative that is implemented for f32, Vec2, Vec3, Vec3A and Vec4. Disallows f64 to be derived, spec expects f32 only. Renamed derivatives from ddx() to dfdx() to better match glsl's dFdx().

Spec section: https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_derivative_instructions

Copy link
Collaborator

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Is there any way we can polyfill for f64?

There appear to be valid CI failures.

use glam::{Vec2, Vec3, Vec3A, Vec4};

#[cfg(target_arch = "spirv")]
macro_rules! cap_deriv_control {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This macro is used elsewhere but impl is commented out?

Copy link
Member Author

@Firestar99 Firestar99 Apr 10, 2025

Choose a reason for hiding this comment

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

I'll remove it for now, can always undo later if you don't squash.

But interestingly, you can use this to enable a capability from the code itself, and do not need to predeclare it in SpirvBuilder. I want to follow this up a little more, cause I would like to get rid of predeclaring them at all, and you just enable features on the things you use implicitly. Note that using that shader (in Vulkan, unsure about wgpu) requires you to explicitly enable said device feature, so you'd still notice when you accidentally were to use too many features.

Refactoring this would completely remove recompilations due to changed predeclared capabilities, which change absolutely nothing anyway and is a blocker for redistributing build scripts with shaders. And could allow shaders within the same crate to only require the capabilities that the shader actually needs, but this needs more research on how DCE is performed on capabilities.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be cool, but commented out it does nothing. So yeah, let's remove and follow-up. I'm a little hesitant that the capabilities code relies on would not be statically known. I think we should take a large hint to how simd is handled in rustc with compile control and runtime checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a commit to just comment it out, now I got one commit that removes it and can be reverted later

@Firestar99
Copy link
Member Author

Forget about polyfiling this for f64 or anything else really. This is a fragment shader only instrinsic for computing derivatives with your neighboring fragments, which are needed in eg. texture operations for mip level selection.

@LegNeato
Copy link
Collaborator

Looks like CI failing:

docs for unsafe trait missing # Safety section

Copy link
Collaborator

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Awesome!

@Firestar99 Firestar99 added this pull request to the merge queue Apr 10, 2025
Merged via the queue into main with commit 0b37696 Apr 10, 2025
7 checks passed
@Firestar99 Firestar99 deleted the derivatives branch April 10, 2025 20:52
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.

3 participants