[naga msl-out hlsl-out] Improve workaround for infinite loops causing undefined behaviour #6929
Conversation
|
Hey @rudderbucky can you try this PR to see how the shader performance is with the workaround in place? |
|
So it turns out I was able to find a couple workarounds to avoid the performance hit... I will give this a whirl around the weekend though, please remind me if I forget @cwfitzgerald |
|
The checks probably need to be moved to the beginning of the loop as mentioned in #6528 (comment) |
Ah of course. I'll update the patch |
… undefined behaviour We must ensure that all loops emitted by the naga backends will terminate, in order to avoid undefined behaviour. This was previously implemented for the msl backend in 6545. However, the usage of `volatile` prevents the compiler from making other important optimizations. This patch improves the msl workaround and additionally implements it for hlsl. The spv implementation will be left for a follow up. Rather than using volatile, this patch increments a counter on every loop iteration, breaking from the loop after 2^64 iterations. This ensures the compiler treats the loop as finite thereby avoiding undefined behaviour, whilst at the same time allowing for other optimizations and in reality not actually affecting execution.
jamienicol
force-pushed
the
infinite-loops-ub
branch
from
565fb7c to
2976158
Compare
I think this may have been the cause of the additional macos failures. At least with the checks at the top of the loop we no longer have any new failures on CI. So I have removed the patch that updated the test expectations |
|
@rudderbucky did you have a chance to test the performance with this PR? |
|
Hi... I tried setting my game up to hit this performance snag but haven't been able to get performance to significantly drop even after disabling a few performance improvements I made around the same time... maybe something else in wgpu 23 helped improve performance again... sorry about that. |
Connections
Partially solves #6572 (only hlsl and msl)
Description
We must ensure that all loops emitted by the naga backends will terminate, in order to avoid undefined behaviour. This was previously implemented for the msl backend in #6545. However, the usage of
volatileprevents the compiler from making other important optimizations. This patch improves the msl workaround and additionally implements it for hlsl. The spv implementation will be left for a follow up.Rather than using volatile, this patch increments a counter on every loop iteration, breaking from the loop after 2^64 iterations. This ensures the compiler treats the loop as finite thereby avoiding undefined behaviour, whilst at the same time allowing for other optimizations and in reality not actually affecting execution.
Testing
Modified the hello_compute example to reproduce the undefined behaviour then ensured this patch avoids it.
Inspected metal and hlsl output in shaderplayground to ensure simple loop unrolling still works as expected
Checklist
cargo fmt.taplo format.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.