[naga hlsl-out msl-out spv-out] Avoid undefined behaviour when casting floats to integers #7422
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Failing test due to a newly discovered bug. So also depends on #7424 |
jamienicol
force-pushed
the
cast-float-to-int-ub
branch
4 times, most recently
from
e7815de to
7a3923d
Compare
…g floats to integers Currently we generate code to convert floating point values to integers using constructor-style casts in HLSL, static_cast in MSL, and OpConvertFToS/OpConvertFToU instructions in SPV. Unfortunately the behaviour of these operations is undefined when the original value is outside of the range of the target type. This patch avoids undefined behaviour by first clamping the value to be inside the target type's range, then performing the cast. Additionally, we specifically clamp to the minimum and maximum values that are exactly representable in both the original and the target type, as per the WGSL spec[1]. Note that these may not be the same as the minimum and maximum values of the target type. We additionally must ensure we clamp in the same manner for conversions during const evaluation. Lastly, although not part of the WGSL spec, we do the same for casting from F64 and/or to I64 or U64. [1] https://www.w3.org/TR/WGSL/#floating-point-conversion
jamienicol
force-pushed
the
cast-float-to-int-ub
branch
from
7a3923d to
1c6ad8a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connections
Depends on #7421 (not functionally, but they touch adjacent code so easier to land that first)
Depends on #7424
Fixes #7386
Fixes #7387
Description
We currently hit undefined behaviour when casting from floats to integers if the value is out of range for the target integer type. This avoids that. Additionally we must ensure we adhere to the spec's requirements:
This can be achieved by
clamp()ing to the minimum and maximum floating point values that are also representable in the target type, before casting.Even though F64, I64, and U64 are not part of the WGSL spec we use the same behaviour when casting to/from those types, as it seems the less surprising choice.
Testing
Added new snapshot tests to ensure we handle this correctly during const eval.
I tested by hand with a compute shader that runtime values were as expected (on Windows/DXC, Linux/Vulkan, and Macos/Metal) but there is nothing in the test suite.
Squash or Rebase?
Either
Checklist
cargo fmt.cargo clippy --tests. If applicable, add:cargo xtask testto run tests.CHANGELOG.mdentry.