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

Debug printf in shaders #4297

Open
wants to merge 17 commits into
base: trunk
Choose a base branch
from
Open

Debug printf in shaders #4297

wants to merge 17 commits into from

Conversation

exrook
Copy link
Contributor

@exrook exrook commented Oct 25, 2023

  • Run cargo clippy.
  • Run cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Migrated from gfx-rs/naga#2563

Description
Exposes the vulkan validation layers shader printf feature to wgsl shaders through a new debugPrintf builtin function.

example:

@compute @workgroup_size(8,1,1)
fn main(@builtin(local_invocation_index) idx: u32) {
    debugPrintf("Hello from %d", idx);
}
output with validation layers enabled and configured properly:
UNASSIGNED-DEBUG-PRINTF(INFO / SPEC): msgNum: -1841738615 - Validation Information: [ UNASSIGNED-DEBUG-PRINTF ] | MessageID = 0x92394c89 | Hello from thread 0! 
    Objects: 0
UNASSIGNED-DEBUG-PRINTF(INFO / SPEC): msgNum: -1841738615 - Validation Information: [ UNASSIGNED-DEBUG-PRINTF ] | MessageID = 0x92394c89 | Hello from thread 1! 
    Objects: 0
UNASSIGNED-DEBUG-PRINTF(INFO / SPEC): msgNum: -1841738615 - Validation Information: [ UNASSIGNED-DEBUG-PRINTF ] | MessageID = 0x92394c89 | Hello from thread 2! 
    Objects: 0
UNASSIGNED-DEBUG-PRINTF(INFO / SPEC): msgNum: -1841738615 - Validation Information: [ UNASSIGNED-DEBUG-PRINTF ] | MessageID = 0x92394c89 | Hello from thread 3! 
    Objects: 0
UNASSIGNED-DEBUG-PRINTF(INFO / SPEC): msgNum: -1841738615 - Validation Information: [ UNASSIGNED-DEBUG-PRINTF ] | MessageID = 0x92394c89 | Hello from thread 4! 
    Objects: 0
UNASSIGNED-DEBUG-PRINTF(INFO / SPEC): msgNum: -1841738615 - Validation Information: [ UNASSIGNED-DEBUG-PRINTF ] | MessageID = 0x92394c89 | Hello from thread 5! 
    Objects: 0
UNASSIGNED-DEBUG-PRINTF(INFO / SPEC): msgNum: -1841738615 - Validation Information: [ UNASSIGNED-DEBUG-PRINTF ] | MessageID = 0x92394c89 | Hello from thread 6! 
    Objects: 0
UNASSIGNED-DEBUG-PRINTF(INFO / SPEC): msgNum: -1841738615 - Validation Information: [ UNASSIGNED-DEBUG-PRINTF ] | MessageID = 0x92394c89 | Hello from thread 7! 
    Objects: 0

This feature is also supported on HLSL/DirectX though only when using FXC. DXC does not fully support printf, see microsoft/hlsl-specs#245.

Additionally supports converting SPIR-V shaders containing NonSemantic.DebugPrintf to wgsl, glsl, hlsl.

Testing
TBD

@exrook
Copy link
Contributor Author

exrook commented Nov 1, 2023

Looking into how the validation layers implement this, there's no reason we couldn't universally support a feature like this in a similar way with a pass on the naga IR + transparently adding a bind group to shader dispatches.

That being said I think this basic validation layer based impl would still be nice to have immediately for users on vulkan platforms.

@exrook exrook force-pushed the debug_printf branch 4 times, most recently from 92fc6a5 to dae4b17 Compare November 2, 2023 12:28
@exrook exrook marked this pull request as ready for review November 2, 2023 23:12
@exrook exrook requested review from a team as code owners November 2, 2023 23:12
@exrook exrook force-pushed the debug_printf branch 2 times, most recently from b6841ff to b4c2a18 Compare November 6, 2023 13:32
@kvark
Copy link
Member

kvark commented Nov 9, 2023

I believe the minimal functionality in WGSL you'd need in order to support this in user space is just a string type. As soon as you have a string literal to work with, you can have WGSL preprocessor recognizing a construct and writing the format string as well as arguments into some debug buffer.

@exrook
Copy link
Contributor Author

exrook commented Nov 9, 2023

I believe the minimal functionality in WGSL you'd need in order to support this in user space is just a string type. As soon as you have a string literal to work with, you can have WGSL preprocessor recognizing a construct and writing the format string as well as arguments into some debug buffer.

The intent behind this PR was to make debugging shaders easier for wgpu users, with this PR if I want to inspect a value all I have to do is enable the validation layers and wgpu feature, and add a single line to my shader source.

Even without string support in the language a user can already implement printf-like behavior in their project, as done here for example, but I think it's a much better experience if we can provide built in debugging utilities, so I can debug my shaders without having to make any changes to my project.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Looking good from wgpu side.

Please re-request a review from me once the changes are addressed to make sure I see it!

wgpu-hal/src/vulkan/instance.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/device.rs Outdated Show resolved Hide resolved
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Approving the wgpu side!

@LegNeato
Copy link
Contributor

Is there anything left to do here besides rebase?

@cwfitzgerald
Copy link
Member

Needs a review from the Naga team, will poke them in our next meeting, but holidays are approaching, so it may be a little bit.

naga/src/front/spv/ext_inst.rs Outdated Show resolved Hide resolved
naga/tests/out/hlsl/debug-printf-s.hlsl Outdated Show resolved Hide resolved
@jimblandy
Copy link
Member

I took the liberty of rebasing the branch on trunk, replacing the merge commit. I hope that's all right.

@jimblandy
Copy link
Member

My rebase seems to have broken something. I'm looking into it.

@exrook exrook force-pushed the debug_printf branch 3 times, most recently from 92bca6b to a970533 Compare December 30, 2023 16:18
@cwfitzgerald
Copy link
Member

@exrook friendly poke :)

@MiniaczQ
Copy link

This looks super helpful!

@exrook
Copy link
Contributor Author

exrook commented Jan 24, 2024

I'll update this tonight :)

@exrook exrook force-pushed the debug_printf branch 3 times, most recently from 9a36c03 to d7b3be9 Compare January 25, 2024 15:30
@MiniaczQ
Copy link

Is this ready for re-review?

@Wumpf Wumpf requested a review from cwfitzgerald May 4, 2024 20:13
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Small nits, just waiting on naga review.

/// Enables support for debugPrintf in WGSL shaders.
///
/// Supported Platforms:
/// - DX11 (fxc only)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// - DX11 (fxc only)

/// - DX11 (fxc only)
/// - DX12 (fxc only)
/// - Vulkan
/// - OpenGL
Copy link
Member

Choose a reason for hiding this comment

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

We don't support it on GL, so we should label this as an "unimplemented platform, if it is implementable

@cwfitzgerald cwfitzgerald requested a review from a team May 9, 2024 03:13
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.

None yet

6 participants