[PATCH v2 0/4] MR289: spirv: Clamp ftou output.
SPIR-V says this is undefined behaviour, but Direct3D actually specifies that it should clamp. Most drivers do clamp, but llvmpipe does not. Hence this fixes a couple of tests with llvmpipe. This does of course add overhead to the ftou instruction, but I cannot imagine that it is a common instruction to execute. This also is not the first time we perform such checks; cf. udiv. -- v2: vkd3d-shader/spirv: Clamp ftoi upper bound to UINT_MAX. vkd3d-shader/spirv: Clamp ftoi lower bound to -INT_MIN. vkd3d-shader/spirv: Clamp ftou upper bound to UINT_MAX. vkd3d-shader/spirv: Clamp ftou lower bound to zero. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 41 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 2725ed80..a04d315b 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -1715,6 +1715,15 @@ static uint32_t vkd3d_spirv_build_op_glsl_std450_cos(struct vkd3d_spirv_builder return vkd3d_spirv_build_op_glsl_std450_tr1(builder, GLSLstd450Cos, result_type, operand); } +static uint32_t vkd3d_spirv_build_op_glsl_std450_max(struct vkd3d_spirv_builder *builder, + uint32_t result_type, uint32_t x, uint32_t y) +{ + uint32_t glsl_std450_id = vkd3d_spirv_get_glsl_std450_instr_set(builder); + uint32_t operands[] = {x, y}; + return vkd3d_spirv_build_op_ext_inst(builder, result_type, glsl_std450_id, + GLSLstd450NMax, operands, ARRAY_SIZE(operands)); +} + static uint32_t vkd3d_spirv_build_op_glsl_std450_nclamp(struct vkd3d_spirv_builder *builder, uint32_t result_type, uint32_t x, uint32_t min, uint32_t max) { @@ -6536,7 +6545,6 @@ static SpvOp spirv_compiler_map_alu_instruction(const struct vkd3d_shader_instru {VKD3DSIH_DTOU, SpvOpConvertFToU}, {VKD3DSIH_FTOD, SpvOpFConvert}, {VKD3DSIH_FTOI, SpvOpConvertFToS}, - {VKD3DSIH_FTOU, SpvOpConvertFToU}, {VKD3DSIH_IADD, SpvOpIAdd}, {VKD3DSIH_INEG, SpvOpSNegate}, {VKD3DSIH_ISHL, SpvOpShiftLeftLogical}, @@ -7002,6 +7010,33 @@ static void spirv_compiler_emit_udiv(struct spirv_compiler *compiler, } } +static void spirv_compiler_emit_ftou(struct spirv_compiler *compiler, + const struct vkd3d_shader_instruction *instruction) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + const struct vkd3d_shader_dst_param *dst = instruction->dst; + const struct vkd3d_shader_src_param *src = instruction->src; + uint32_t src_type_id, type_id, src_id, zero_id, val_id; + + assert(instruction->dst_count == 1); + assert(instruction->src_count == 1); + + /* OpConvertFToU has undefined results if the result cannot be represented + * as an unsigned integer, but Direct3D expects the result to saturate, + * and for NaN to yield zero. */ + + src_type_id = spirv_compiler_get_type_id_for_reg(compiler, &src->reg, dst->write_mask); + type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); + src_id = spirv_compiler_emit_load_src(compiler, src, dst->write_mask); + zero_id = spirv_compiler_get_constant_float_vector(compiler, 0.0f, + vkd3d_write_mask_component_count(dst->write_mask)); + + val_id = vkd3d_spirv_build_op_glsl_std450_max(builder, src_type_id, src_id, zero_id); + val_id = vkd3d_spirv_build_op_tr1(builder, &builder->function_stream, SpvOpConvertFToU, type_id, val_id); + + spirv_compiler_emit_store_dst(compiler, dst, val_id); +} + static void spirv_compiler_emit_bitfield_instruction(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { @@ -9286,7 +9321,6 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_DTOU: case VKD3DSIH_FTOD: case VKD3DSIH_FTOI: - case VKD3DSIH_FTOU: case VKD3DSIH_IADD: case VKD3DSIH_INEG: case VKD3DSIH_ISHL: @@ -9347,6 +9381,9 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_UDIV: spirv_compiler_emit_udiv(compiler, instruction); break; + case VKD3DSIH_FTOU: + spirv_compiler_emit_ftou(compiler, instruction); + break; case VKD3DSIH_DEQ: case VKD3DSIH_DGE: case VKD3DSIH_DLT: -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index a04d315b..e5da4bec 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -7013,10 +7013,12 @@ static void spirv_compiler_emit_udiv(struct spirv_compiler *compiler, static void spirv_compiler_emit_ftou(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { + uint32_t src_id, zero_id, uint_max_id, float_max_id, condition_id, val_id; struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; const struct vkd3d_shader_dst_param *dst = instruction->dst; const struct vkd3d_shader_src_param *src = instruction->src; - uint32_t src_type_id, type_id, src_id, zero_id, val_id; + uint32_t src_type_id, dst_type_id, condition_type_id; + unsigned int component_count; assert(instruction->dst_count == 1); assert(instruction->src_count == 1); @@ -7025,14 +7027,22 @@ static void spirv_compiler_emit_ftou(struct spirv_compiler *compiler, * as an unsigned integer, but Direct3D expects the result to saturate, * and for NaN to yield zero. */ + component_count = vkd3d_write_mask_component_count(dst->write_mask); src_type_id = spirv_compiler_get_type_id_for_reg(compiler, &src->reg, dst->write_mask); - type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); + dst_type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); src_id = spirv_compiler_emit_load_src(compiler, src, dst->write_mask); - zero_id = spirv_compiler_get_constant_float_vector(compiler, 0.0f, - vkd3d_write_mask_component_count(dst->write_mask)); + zero_id = spirv_compiler_get_constant_float_vector(compiler, 0.0f, component_count); val_id = vkd3d_spirv_build_op_glsl_std450_max(builder, src_type_id, src_id, zero_id); - val_id = vkd3d_spirv_build_op_tr1(builder, &builder->function_stream, SpvOpConvertFToU, type_id, val_id); + + float_max_id = spirv_compiler_get_constant_float_vector(compiler, 4294967296.0f, component_count); + uint_max_id = spirv_compiler_get_constant_uint_vector(compiler, UINT_MAX, component_count); + condition_type_id = vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_BOOL, component_count); + condition_id = vkd3d_spirv_build_op_tr2(builder, &builder->function_stream, + SpvOpFOrdGreaterThanEqual, condition_type_id, val_id, float_max_id); + + val_id = vkd3d_spirv_build_op_tr1(builder, &builder->function_stream, SpvOpConvertFToU, dst_type_id, val_id); + val_id = vkd3d_spirv_build_op_select(builder, dst_type_id, condition_id, uint_max_id, val_id); spirv_compiler_emit_store_dst(compiler, dst, val_id); } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 32 ++++++++++++++++++++++++++++++-- tests/d3d12.c | 2 ++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index e5da4bec..e83c34fc 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -6544,7 +6544,6 @@ static SpvOp spirv_compiler_map_alu_instruction(const struct vkd3d_shader_instru {VKD3DSIH_DTOI, SpvOpConvertFToS}, {VKD3DSIH_DTOU, SpvOpConvertFToU}, {VKD3DSIH_FTOD, SpvOpFConvert}, - {VKD3DSIH_FTOI, SpvOpConvertFToS}, {VKD3DSIH_IADD, SpvOpIAdd}, {VKD3DSIH_INEG, SpvOpSNegate}, {VKD3DSIH_ISHL, SpvOpShiftLeftLogical}, @@ -7010,6 +7009,33 @@ static void spirv_compiler_emit_udiv(struct spirv_compiler *compiler, } } +static void spirv_compiler_emit_ftoi(struct spirv_compiler *compiler, + const struct vkd3d_shader_instruction *instruction) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + const struct vkd3d_shader_dst_param *dst = instruction->dst; + const struct vkd3d_shader_src_param *src = instruction->src; + uint32_t src_type_id, type_id, src_id, int_min_id, val_id; + + assert(instruction->dst_count == 1); + assert(instruction->src_count == 1); + + /* OpConvertFToI has undefined results if the result cannot be represented + * as a signed integer, but Direct3D expects the result to saturate, + * and for NaN to yield zero. */ + + src_type_id = spirv_compiler_get_type_id_for_reg(compiler, &src->reg, dst->write_mask); + type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); + src_id = spirv_compiler_emit_load_src(compiler, src, dst->write_mask); + int_min_id = spirv_compiler_get_constant_float_vector(compiler, -2147483648.0f, + vkd3d_write_mask_component_count(dst->write_mask)); + + val_id = vkd3d_spirv_build_op_glsl_std450_max(builder, src_type_id, src_id, int_min_id); + val_id = vkd3d_spirv_build_op_tr1(builder, &builder->function_stream, SpvOpConvertFToS, type_id, val_id); + + spirv_compiler_emit_store_dst(compiler, dst, val_id); +} + static void spirv_compiler_emit_ftou(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { @@ -9330,7 +9356,6 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_DTOI: case VKD3DSIH_DTOU: case VKD3DSIH_FTOD: - case VKD3DSIH_FTOI: case VKD3DSIH_IADD: case VKD3DSIH_INEG: case VKD3DSIH_ISHL: @@ -9391,6 +9416,9 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_UDIV: spirv_compiler_emit_udiv(compiler, instruction); break; + case VKD3DSIH_FTOI: + spirv_compiler_emit_ftoi(compiler, instruction); + break; case VKD3DSIH_FTOU: spirv_compiler_emit_ftou(compiler, instruction); break; diff --git a/tests/d3d12.c b/tests/d3d12.c index af27cd2e..0474abc9 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -36736,6 +36736,8 @@ START_TEST(d3d12) pfn_D3D12CreateVersionedRootSignatureDeserializer = get_d3d12_pfn(D3D12CreateVersionedRootSignatureDeserializer); pfn_D3D12SerializeVersionedRootSignature = get_d3d12_pfn(D3D12SerializeVersionedRootSignature); + run_test(test_shader_instructions);return; + run_test(test_create_device); run_test(test_node_count); run_test(test_check_feature_support); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index e83c34fc..daf830fb 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2895,6 +2895,12 @@ static uint32_t spirv_compiler_get_constant_vector(struct spirv_compiler *compil return spirv_compiler_get_constant(compiler, component_type, component_count, values); } +static uint32_t spirv_compiler_get_constant_int_vector(struct spirv_compiler *compiler, + uint32_t value, unsigned int component_count) +{ + return spirv_compiler_get_constant_vector(compiler, VKD3D_SHADER_COMPONENT_INT, component_count, value); +} + static uint32_t spirv_compiler_get_constant_uint_vector(struct spirv_compiler *compiler, uint32_t value, unsigned int component_count) { @@ -7012,10 +7018,12 @@ static void spirv_compiler_emit_udiv(struct spirv_compiler *compiler, static void spirv_compiler_emit_ftoi(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { + uint32_t src_id, int_min_id, int_max_id, float_max_id, condition_id, val_id; struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; const struct vkd3d_shader_dst_param *dst = instruction->dst; const struct vkd3d_shader_src_param *src = instruction->src; - uint32_t src_type_id, type_id, src_id, int_min_id, val_id; + uint32_t src_type_id, dst_type_id, condition_type_id; + unsigned int component_count; assert(instruction->dst_count == 1); assert(instruction->src_count == 1); @@ -7024,14 +7032,22 @@ static void spirv_compiler_emit_ftoi(struct spirv_compiler *compiler, * as a signed integer, but Direct3D expects the result to saturate, * and for NaN to yield zero. */ + component_count = vkd3d_write_mask_component_count(dst->write_mask); src_type_id = spirv_compiler_get_type_id_for_reg(compiler, &src->reg, dst->write_mask); - type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); + dst_type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); src_id = spirv_compiler_emit_load_src(compiler, src, dst->write_mask); - int_min_id = spirv_compiler_get_constant_float_vector(compiler, -2147483648.0f, - vkd3d_write_mask_component_count(dst->write_mask)); + int_min_id = spirv_compiler_get_constant_float_vector(compiler, -2147483648.0f, component_count); val_id = vkd3d_spirv_build_op_glsl_std450_max(builder, src_type_id, src_id, int_min_id); - val_id = vkd3d_spirv_build_op_tr1(builder, &builder->function_stream, SpvOpConvertFToS, type_id, val_id); + + float_max_id = spirv_compiler_get_constant_float_vector(compiler, 2147483648.0f, component_count); + int_max_id = spirv_compiler_get_constant_int_vector(compiler, INT_MAX, component_count); + condition_type_id = vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_BOOL, component_count); + condition_id = vkd3d_spirv_build_op_tr2(builder, &builder->function_stream, + SpvOpFOrdGreaterThanEqual, condition_type_id, val_id, float_max_id); + + val_id = vkd3d_spirv_build_op_tr1(builder, &builder->function_stream, SpvOpConvertFToS, dst_type_id, val_id); + val_id = vkd3d_spirv_build_op_select(builder, dst_type_id, condition_id, int_max_id, val_id); spirv_compiler_emit_store_dst(compiler, dst, val_id); } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289
I've added ftoi clamping to this patch series. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289#note_41286
On Thu Aug 3 09:30:54 2023 +0000, Giovanni Mascellani wrote:
It's worse than that; we'd have to perform a branch *per component*. Right. Though on second thought you can use `OpSelect` as you did, provided that you first mask away the invalid inputs with another `OpSelect`. `OpConvertFToU` doesn't require the result to have the same width as the input, so another option is `NClamp` to `(0.0f, 4294967296.0f)`, convert to uint64, clamp to 0xffffffff, then truncate. `NClamp` does `max` first, which takes care of `NaN`.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289#note_41295
On Fri Aug 4 02:46:35 2023 +0000, Conor McCarthy wrote:
`OpConvertFToU` doesn't require the result to have the same width as the input, so another option is `NClamp` to `(0.0f, 4294967296.0f)`, convert to uint64, clamp to 0xffffffff, then truncate. `NClamp` does `max` first, which takes care of `NaN`. That does require int64 support, unfortunately.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289#note_41296
On Fri Aug 4 03:16:58 2023 +0000, Zebediah Figura wrote:
That does require int64 support, unfortunately. We will eventually need a 16-bit version for `half`, and I may as well write it that way, and then the same path can be used for `float` if int64 is supported.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289#note_41298
A couple of mistakes in commit messages: for `ftoi` you're clamping to `INT_MIN` (not `-INT_MIN`, which in a sense is the same thing, but just `INT_MIN` looks more appropriate) and `INT_MAX` (not `UINT_MAX`). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289#note_44149
Giovanni Mascellani (@giomasce) commented about tests/d3d12.c:
pfn_D3D12CreateVersionedRootSignatureDeserializer = get_d3d12_pfn(D3D12CreateVersionedRootSignatureDeserializer); pfn_D3D12SerializeVersionedRootSignature = get_d3d12_pfn(D3D12SerializeVersionedRootSignature);
+ run_test(test_shader_instructions);return;
I don't think that's supposed to be here... :-P -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289#note_44147
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/spirv.c:
* as a signed integer, but Direct3D expects the result to saturate, * and for NaN to yield zero. */
+ component_count = vkd3d_write_mask_component_count(dst->write_mask); src_type_id = spirv_compiler_get_type_id_for_reg(compiler, &src->reg, dst->write_mask); - type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); + dst_type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); src_id = spirv_compiler_emit_load_src(compiler, src, dst->write_mask); - int_min_id = spirv_compiler_get_constant_float_vector(compiler, -2147483648.0f, - vkd3d_write_mask_component_count(dst->write_mask));
+ int_min_id = spirv_compiler_get_constant_float_vector(compiler, -2147483648.0f, component_count); val_id = vkd3d_spirv_build_op_glsl_std450_max(builder, src_type_id, src_id, int_min_id);
Here you're not mapping NaN to zero, but rather to `INT_MIN`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289#note_44148
Here you're not mapping NaN to zero, but rather to `INT_MIN`.
Yeah. I wrote the comment for documentation purposes, then left off the part where NaN is actually flushed to zero. I should just add a fifth commit to actually perform that flush. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/289#note_44281
participants (4)
-
Conor McCarthy (＠cmccarthy) -
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)