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

[naga spv-out] Ensure loops generated by SPIRV backend are bounded #7080

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

jamienicol
Copy link
Contributor

@jamienicol jamienicol commented Feb 7, 2025

Connections
Partial fix for #6572 (can tick SPIR-V off, still GLSL to go)
Related to #6929: that updated the previous MSL workaround to use the 64bit counter approach, and added the workaround for HLSL

Description
If it is undefined behaviour for loops to be infinite, then, when encountering an infinite loop, downstream compilers are able to make certain optimizations that may be unsafe. For example, omitting bounds checks. To prevent this, we must ensure that any loops emitted by our backends are provably bounded. We already do this for both the MSL and HLSL backends. This patch makes us do so for SPIRV as well.

The construct used is the same as for HLSL and MSL backends: use a vec2 to emulate a 64-bit counter, which is incremented every iteration and breaks after 2^64 iterations.

While the implementation is fairly verbose for the SPIRV backend, the logic is simple enough. The one point of note is that SPIRV requires OpVariable instructions with a Function storage class to be located at the start of the first block of the function. We must therefore do an initial pass over the function to generate the IDs used for the loop counter variables, and ensure the corresponding OpVariable instructions are emitted at the start of the function. Then finally during the main code-generation pass we can refer to these IDs.

As this may negatively impact performance, this workaround can be disabled using the same mechanism as for other backends: eg calling Device::create_shader_module_trusted() and setting the ShaderRuntimeChecks::force_loop_bounding flag to false.

Testing
Inspected snapshot test changes. Ensured validation still passes

Checklist

@jamienicol jamienicol requested a review from a team as a code owner February 7, 2025 13:36
@jamienicol jamienicol force-pushed the loop-bounds-spv branch 2 times, most recently from ee4b7ea to ea7babd Compare February 11, 2025 09:28
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

We think there might be a nicer way to generate the temporary variables as we go, rather than making a separate pass over them beforehand.

@jamienicol
Copy link
Contributor Author

We think there might be a nicer way to generate the temporary variables as we go, rather than making a separate pass over them beforehand.

Indeed, the pass to generate a Function's body, ie Function::write_function_body() already occurs prior to Function::to_words() - which writes instructions for the local variables etc, and then writes the body. So there's no need for yet another pass.

Patch updated

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Looks great!

@teoxoy
Copy link
Member

teoxoy commented Feb 25, 2025

@jamienicol looks like there are some conflicts now

If it is undefined behaviour for loops to be infinite, then, when
encountering an infinite loop, downstream compilers are able to make
certain optimizations that may be unsafe. For example, omitting bounds
checks. To prevent this, we must ensure that any loops emitted by our
backends are provably bounded. We already do this for both the MSL and
HLSL backends. This patch makes us do so for SPIRV as well.

The construct used is the same as for HLSL and MSL backends: use a
vec2<u32> to emulate a 64-bit counter, which is incremented every
iteration and breaks after 2^64 iterations.

While the implementation is fairly verbose for the SPIRV backend, the
logic is simple enough. The one point of note is that SPIRV requires
`OpVariable` instructions with a `Function` storage class to be
located at the start of the first block of the function. We therefore
remember the IDs generated for each loop counter variable in a
function whilst generating the function body's code. The instructions
to declare these variables are then emitted in `Function::to_words()`
prior to emitting the function's body.

As this may negatively impact shader performance, this workaround can
be disabled using the same mechanism as for other backends: eg calling
Device::create_shader_module_trusted() and setting the
ShaderRuntimeChecks::force_loop_bounding flag to false.
@teoxoy teoxoy dismissed jimblandy’s stale review February 25, 2025 14:23

Feedback was addressed

@teoxoy teoxoy merged commit b7d1f4c into gfx-rs:trunk Feb 25, 2025
35 checks passed
@jamienicol jamienicol deleted the loop-bounds-spv branch February 25, 2025 14:25
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.

5 participants