[PATCH v2 0/2] MR338: vkd3d-shader/hlsl: Correctly fold casts from float.
-- v2: vkd3d-shader/hlsl: Correctly fold casts from double. vkd3d-shader/hlsl: Correctly fold casts from float. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/338
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> I.e., without invoking undefined behavior in the compiler. The rules are desumed from the the MSDN documentation for ftoi and ftou. --- libs/vkd3d-shader/hlsl_constant_ops.c | 29 +++++++++++++++++++++++++-- tests/hlsl/cast-to-uint.shader_test | 2 +- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_constant_ops.c b/libs/vkd3d-shader/hlsl_constant_ops.c index 41a72ab6c..eebbac9ba 100644 --- a/libs/vkd3d-shader/hlsl_constant_ops.c +++ b/libs/vkd3d-shader/hlsl_constant_ops.c @@ -63,6 +63,31 @@ static bool fold_abs(struct hlsl_ctx *ctx, struct hlsl_constant_value *dst, return true; } +static uint32_t float_to_uint(float x) +{ + if (isnan(x) || x <= 0) + return 0; + + if (x >= 4294967296.0f) + return UINT32_MAX; + + return x; +} + +static int32_t float_to_int(float x) +{ + if (isnan(x)) + return 0; + + if (x <= -2147483648.0f) + return INT32_MIN; + + if (x >= 2147483648.0f) + return INT32_MAX; + + return x; +} + static bool fold_cast(struct hlsl_ctx *ctx, struct hlsl_constant_value *dst, const struct hlsl_type *dst_type, const struct hlsl_ir_constant *src) { @@ -86,8 +111,8 @@ static bool fold_cast(struct hlsl_ctx *ctx, struct hlsl_constant_value *dst, { case HLSL_TYPE_FLOAT: case HLSL_TYPE_HALF: - u = src->value.u[k].f; - i = src->value.u[k].f; + u = float_to_uint(src->value.u[k].f); + i = float_to_int(src->value.u[k].f); f = src->value.u[k].f; d = src->value.u[k].f; break; diff --git a/tests/hlsl/cast-to-uint.shader_test b/tests/hlsl/cast-to-uint.shader_test index 4ffc041ad..93862a36f 100644 --- a/tests/hlsl/cast-to-uint.shader_test +++ b/tests/hlsl/cast-to-uint.shader_test @@ -41,4 +41,4 @@ float4 main() : sv_target [test] draw quad -todo probe all rgba (0.5, 0.5, 0.5, 0.5) +probe all rgba (0.5, 0.5, 0.5, 0.5) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/338
From: Giovanni Mascellani <giovanni(a)mascellani.eu> --- libs/vkd3d-shader/hlsl_constant_ops.c | 29 +++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_constant_ops.c b/libs/vkd3d-shader/hlsl_constant_ops.c index eebbac9ba..9a2911909 100644 --- a/libs/vkd3d-shader/hlsl_constant_ops.c +++ b/libs/vkd3d-shader/hlsl_constant_ops.c @@ -88,6 +88,31 @@ static int32_t float_to_int(float x) return x; } +static uint32_t double_to_uint(double x) +{ + if (isnan(x) || x <= 0) + return 0; + + if (x >= 4294967296.0) + return UINT32_MAX; + + return x; +} + +static int32_t double_to_int(double x) +{ + if (isnan(x)) + return 0; + + if (x <= -2147483648.0) + return INT32_MIN; + + if (x >= 2147483648.0) + return INT32_MAX; + + return x; +} + static bool fold_cast(struct hlsl_ctx *ctx, struct hlsl_constant_value *dst, const struct hlsl_type *dst_type, const struct hlsl_ir_constant *src) { @@ -118,8 +143,8 @@ static bool fold_cast(struct hlsl_ctx *ctx, struct hlsl_constant_value *dst, break; case HLSL_TYPE_DOUBLE: - u = src->value.u[k].d; - i = src->value.u[k].d; + u = double_to_uint(src->value.u[k].d); + i = double_to_int(src->value.u[k].d); f = src->value.u[k].d; d = src->value.u[k].d; break; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/338
On Tue Sep 12 12:06:05 2023 +0000, Zebediah Figura wrote:
Oof, I wrote these too, but you beat me to it :x I'd remove the "fold" from the function names, since they're not folding anything by themselves. (Also, "float_to_uint" instead of "float_to_unsigned"?) This doesn't deal with double comparisons. I think you can reuse the same functions by changing their arguments to doubles. The explicit casts seem superfluous. Thanks. Everything should have been dealt with.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/338#note_44999
This seems correct to me. Note that there are transformations from double to float that still are UB. I couldn't find documentation on those. I suspected that the use of `<=` and `>=` combined with the transformation from decimal to binary at compile time of `-2147483648.0`, `2147483648.0`, and `4294967296.0` could include more values that the intended in the checks, but I see now that this is not the case since both float and double can exactly represent these values. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/338#note_45008
This merge request was approved by Francisco Casas. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/338
Note that there are transformations from double to float that still are UB. I couldn't find documentation on those.
Hrm, you're right. From the C99 draft spec § 6.3.1.5.2: "When a double is demoted to float, [... i]f the value being converted can be represented exactly in the new type, it is unchanged. If the value being converted is in the range of values that can be represented but cannot be represented exactly, the result is either the nearest higher or nearest lower representable value, chosen in an implementation-defined manner. If the value being converted is outside the range of values that can be represented, the behavior is undefined." That need not be in scope for this series, though. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/338#note_45031
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/338
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/338
participants (5)
-
Francisco Casas (＠fcasas) -
Giovanni Mascellani
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)