[PATCH v2 0/6] MR694: vkd3d-shader/hlsl: Improve SM1 support for non-float operations, part 2.
Patches with tests and other miscellaneous changes required before the remaining features in my [sm1_logic](https://gitlab.winehq.org/fcasas/vkd3d/-/commits/sm1_logic) branch. Includes some rebased spirv.c patches by Zeb.
-- v2: vkd3d-shader/spirv: Implement SLT and SGE. vkd3d-shader/spirv: Implement CMP. tests: Check that -0.0f is not less than 0.0f. tests: Test comparison and logic operators on SM1 vertex shaders. vkd3d-shader/hlsl: Cast slt before multiplying on ternary operator.
From: Francisco Casas fcasas@codeweavers.com
--- tests/shader_runner_d3d11.c | 1 + tests/shader_runner_d3d9.c | 4 ++++ 2 files changed, 5 insertions(+)
diff --git a/tests/shader_runner_d3d11.c b/tests/shader_runner_d3d11.c index 528c04e9d..26ce81ed7 100644 --- a/tests/shader_runner_d3d11.c +++ b/tests/shader_runner_d3d11.c @@ -620,6 +620,7 @@ static bool d3d11_runner_draw(struct shader_runner *r, { cb = create_buffer(device, D3D11_BIND_CONSTANT_BUFFER, runner->r.uniform_count * sizeof(*runner->r.uniforms), 0, runner->r.uniforms); + ID3D11DeviceContext_VSSetConstantBuffers(context, 0, 1, &cb); ID3D11DeviceContext_PSSetConstantBuffers(context, 0, 1, &cb); }
diff --git a/tests/shader_runner_d3d9.c b/tests/shader_runner_d3d9.c index 4ddb8ffe2..e4e1c6435 100644 --- a/tests/shader_runner_d3d9.c +++ b/tests/shader_runner_d3d9.c @@ -345,6 +345,10 @@ static bool d3d9_runner_draw(struct shader_runner *r,
if (runner->r.uniform_count) { + hr = IDirect3DDevice9_SetVertexShaderConstantF(device, 0, + (const float *)runner->r.uniforms, runner->r.uniform_count / 4); + ok(hr == D3D_OK, "Failed to set uniforms, hr %#lx.\n", hr); + hr = IDirect3DDevice9_SetPixelShaderConstantF(device, 0, (const float *)runner->r.uniforms, runner->r.uniform_count / 4); ok(hr == D3D_OK, "Failed to set uniforms, hr %#lx.\n", hr);
From: Francisco Casas fcasas@codeweavers.com
Otherwise we may get a failing "hlsl_types_are_equal(arg1->data_type, arg2->data_type)" assertion on hlsl_new_binary_expr() when creating the MUL.
This happens in the test introducing in the following patch:
int a, b, c;
void main(out float4 res : COLOR1, in float4 pos : position, out float4 out_pos : sv_position) { out_pos = pos;
res = a ? b/1000.0 : c/1000.0; } --- libs/vkd3d-shader/hlsl_codegen.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 8434a921a..9eb65dc01 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2948,7 +2948,7 @@ static bool lower_ternary(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, stru } else if (ctx->profile->major_version < 4 && ctx->profile->type == VKD3D_SHADER_TYPE_VERTEX) { - struct hlsl_ir_node *neg, *slt, *sum, *mul, *cond2; + struct hlsl_ir_node *neg, *slt, *sum, *cond2, *slt_cast, *mul;
/* Expression used here is "slt(<cond>) * (first - second) + second". */
@@ -2980,7 +2980,11 @@ static bool lower_ternary(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, stru return false; hlsl_block_add_instr(block, sum);
- if (!(mul = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, slt, sum))) + if (!(slt_cast = hlsl_new_cast(ctx, slt, sum->data_type, &instr->loc))) + return false; + hlsl_block_add_instr(block, slt_cast); + + if (!(mul = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, slt_cast, sum))) return false; hlsl_block_add_instr(block, mul);
From: Francisco Casas fcasas@codeweavers.com
Currently these are failing for SM1 because VKD3DSIH_SLT is not being handled by spirv.c. --- Makefile.am | 1 + tests/hlsl/vertex-shader-ops.shader_test | 95 ++++++++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 tests/hlsl/vertex-shader-ops.shader_test
diff --git a/Makefile.am b/Makefile.am index b963e4c03..6dc7623a2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -212,6 +212,7 @@ vkd3d_shader_tests = \ tests/hlsl/uniform-semantics.shader_test \ tests/hlsl/vector-indexing-uniform.shader_test \ tests/hlsl/vector-indexing.shader_test \ + tests/hlsl/vertex-shader-ops.shader_test \ tests/hlsl/writemask-assignop-0.shader_test \ tests/hlsl/writemask-assignop-1.shader_test \ tests/hlsl/writemask-assignop-2.shader_test \ diff --git a/tests/hlsl/vertex-shader-ops.shader_test b/tests/hlsl/vertex-shader-ops.shader_test new file mode 100644 index 000000000..428808f1b --- /dev/null +++ b/tests/hlsl/vertex-shader-ops.shader_test @@ -0,0 +1,95 @@ +% The main objective of this test is to see if operations for SM1 vertex shaders work, because they +% have a different instruction set than pixel shaders. + +% Considerations so that the vertex shader output matches the pixel shader input properly on all +% backends: +% - sv_position must be the last field in the output signature. +% - the d3d9 backend, on Windows, clamps the value of the COLORX semantics before passing it to +% the pixel shader, so their values must be between 0.0 and 1.0. + +[pixel shader] +float4 main(in float4 res : COLOR1) : sv_target +{ + return res; +} + + +[vertex shader todo(sm<4)] +int a, b; + +void main(out float4 res : COLOR1, in float4 pos : position, out float4 out_pos : sv_position) +{ + out_pos = pos; + + res.x = a == b; + res.y = a != b; + res.z = a > b; + res.w = b >= a; +} + +[test] +if(sm<4) uniform 0 float 3 +if(sm<4) uniform 4 float 4 +if(sm>=4) uniform 0 int4 3 4 0 0 +todo(sm<4) draw quad +probe all rgba (0.0, 1.0, 0.0, 1.0) +if(sm<4) uniform 0 float -2 +if(sm<4) uniform 4 float -2 +if(sm>=4) uniform 0 int4 -2 -2 0 0 +todo(sm<4) draw quad +probe all rgba (1.0, 0.0, 0.0, 1.0) + + +[vertex shader todo(sm<4)] +int a, b; + +void main(out float4 res : COLOR1, in float4 pos : position, out float4 out_pos : sv_position) +{ + out_pos = pos; + + res.x = a && b; + res.y = a || b; + res.z = b && a; + res.w = b || a; +} + +[test] +if(sm<4) uniform 0 float 0 +if(sm<4) uniform 4 float 2 +if(sm>=4) uniform 0 int4 0 2 0 0 +todo(sm<4) draw quad +probe all rgba (0.0, 1.0, 0.0, 1.0) +if(sm<4) uniform 0 float -2 +if(sm<4) uniform 4 float 8 +if(sm>=4) uniform 0 int4 -2 8 0 0 +todo(sm<4) draw quad +probe all rgba (1.0, 1.0, 1.0, 1.0) + + +[require] +% The ternary operator works differently in sm6. See sm6-ternary.shader_test. +shader model < 6.0 + +[vertex shader todo(sm<4)] +int a, b, c; + +void main(out float4 res : COLOR1, in float4 pos : position, out float4 out_pos : sv_position) +{ + out_pos = pos; + + res = a ? b/1000.0 : c/1000.0; +} + +[test] +if(sm<4) uniform 0 float 0 +if(sm<4) uniform 4 float 100 +if(sm<4) uniform 8 float 200 +if(sm>=4) uniform 0 int4 0 100 200 0 +todo(sm<4) draw quad +probe all rgba (0.2, 0.2, 0.2, 0.2) +if(sm<4) uniform 0 float -4 +if(sm<4) uniform 4 float 100 +if(sm<4) uniform 8 float 200 +if(sm>=4) uniform 0 int4 -4 100 200 0 +todo(sm<4) draw quad +probe all rgba (0.1, 0.1, 0.1, 0.1)
From: Francisco Casas fcasas@codeweavers.com
--- tests/hlsl/float-comparison.shader_test | 15 +++++++++++++++ tests/hlsl/vertex-shader-ops.shader_test | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+)
diff --git a/tests/hlsl/float-comparison.shader_test b/tests/hlsl/float-comparison.shader_test index b66543443..48e7c4872 100644 --- a/tests/hlsl/float-comparison.shader_test +++ b/tests/hlsl/float-comparison.shader_test @@ -1,3 +1,18 @@ +% Check that -0.0f is not less than 0.0f +[pixel shader todo(sm<4)] +float a; + +float4 main() : sv_target +{ + return -0.0f < a; +} + +[test] +uniform 0 float 0.0 +todo(sm<4) draw quad +probe all rgba (0.0, 0.0, 0.0, 0.0) + + [pixel shader todo(sm<4)] uniform float4 f;
diff --git a/tests/hlsl/vertex-shader-ops.shader_test b/tests/hlsl/vertex-shader-ops.shader_test index 428808f1b..34d496727 100644 --- a/tests/hlsl/vertex-shader-ops.shader_test +++ b/tests/hlsl/vertex-shader-ops.shader_test @@ -14,6 +14,24 @@ float4 main(in float4 res : COLOR1) : sv_target }
+% Check that -0.0f is not less than 0.0f +[vertex shader todo(sm<4)] +float a; + +void main(out float4 res : COLOR1, in float4 pos : position, out float4 out_pos : sv_position) +{ + out_pos = pos; + + res = -0.0f < a; +} + +[test] +if(sm<4) uniform 0 float 0.0 +if(sm>=4) uniform 0 float4 0.0 0.0 0.0 0.0 +todo(sm<4) draw quad +probe all rgba (0.0, 0.0, 0.0, 0.0) + + [vertex shader todo(sm<4)] int a, b;
From: Zebediah Figura zfigura@codeweavers.com
---
Zeb originally wrote a comment with the concern on how this handles the comparisn between -0.0f and 0.0f, but I tested that. --- libs/vkd3d-shader/spirv.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index c0cbdccf6..c8acc73dc 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -7173,8 +7173,15 @@ static void spirv_compiler_emit_movc(struct spirv_compiler *compiler, type_id = spirv_compiler_get_type_id_for_dst(compiler, dst);
if (src[0].reg.data_type != VKD3D_DATA_BOOL) - condition_id = spirv_compiler_emit_int_to_bool(compiler, - VKD3D_SHADER_CONDITIONAL_OP_NZ, src[0].reg.data_type, component_count, condition_id); + { + if (instruction->handler_idx == VKD3DSIH_CMP) + condition_id = vkd3d_spirv_build_op_tr2(builder, &builder->function_stream, SpvOpFOrdGreaterThanEqual, + vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_BOOL, component_count), condition_id, + spirv_compiler_get_constant_float_vector(compiler, 0.0f, component_count)); + else + condition_id = spirv_compiler_emit_int_to_bool(compiler, + VKD3D_SHADER_CONDITIONAL_OP_NZ, src[0].reg.data_type, component_count, condition_id); + } val_id = vkd3d_spirv_build_op_select(builder, type_id, condition_id, src1_id, src2_id);
spirv_compiler_emit_store_dst(compiler, dst, val_id); @@ -9551,6 +9558,7 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, break; case VKD3DSIH_DMOVC: case VKD3DSIH_MOVC: + case VKD3DSIH_CMP: spirv_compiler_emit_movc(compiler, instruction); break; case VKD3DSIH_SWAPC:
From: Zebediah Figura zfigura@codeweavers.com
---
Note that I set the signedness argument to false when rebasing this patch, which I think is correct, but it wouldn't be bad to be sure. --- libs/vkd3d-shader/spirv.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index c8acc73dc..14d40ad81 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -7693,6 +7693,36 @@ static void spirv_compiler_emit_comparison_instruction(struct spirv_compiler *co spirv_compiler_emit_store_reg(compiler, &dst->reg, dst->write_mask, result_id); }
+static void spirv_compiler_emit_float_comparison_instruction(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 src0_id, src1_id, type_id, result_id; + unsigned int component_count; + SpvOp op; + + switch (instruction->handler_idx) + { + case VKD3DSIH_SLT: op = SpvOpFOrdLessThan; break; + case VKD3DSIH_SGE: op = SpvOpFOrdGreaterThanEqual; break; + default: + vkd3d_unreachable(); + } + + component_count = vsir_write_mask_component_count(dst->write_mask); + + src0_id = spirv_compiler_emit_load_src(compiler, &src[0], dst->write_mask); + src1_id = spirv_compiler_emit_load_src(compiler, &src[1], dst->write_mask); + + type_id = vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_BOOL, component_count); + result_id = vkd3d_spirv_build_op_tr2(builder, &builder->function_stream, op, type_id, src0_id, src1_id); + + result_id = spirv_compiler_emit_bool_to_float(compiler, component_count, result_id, false); + spirv_compiler_emit_store_reg(compiler, &dst->reg, dst->write_mask, result_id); +} + static uint32_t spirv_compiler_emit_conditional_branch(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction, uint32_t target_block_id) { @@ -9679,6 +9709,10 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_ULT: spirv_compiler_emit_comparison_instruction(compiler, instruction); break; + case VKD3DSIH_SLT: + case VKD3DSIH_SGE: + spirv_compiler_emit_float_comparison_instruction(compiler, instruction); + break; case VKD3DSIH_BFI: case VKD3DSIH_IBFE: case VKD3DSIH_UBFE:
On Thu Mar 7 18:58:35 2024 +0000, Zebediah Figura wrote:
2/6 seems to have a stray FIXME in it. I think 5/6 and 6/6 are fine but per discussion we may want them to be earlier lowering passes anyway? Note also you'll need to manually accept this one since it has my commits in it.
+% The main objective of this test is to see if operations for SM1
vertex shaders work.
+% Because they have a different instruction set that pixel shaders.
Dependent clauses should be separated by a comma, and "that" seems to be a typographical error.
It makes sense to implement 5/6 and 6/6 in ir.c, in the same spirit as instruction_array_lower_texkills(). I will work on doing that but in the mean time I fixed the other details.
This merge request was approved by Zebediah Figura.
On Thu Mar 7 18:58:34 2024 +0000, Francisco Casas wrote:
It makes sense to implement 5/6 and 6/6 in ir.c, in the same spirit as instruction_array_lower_texkills(). I will work on doing that but in the mean time I fixed the other details.
After some checking, it seems that it is currently common in spirv.c to perform the casts inside the functions that handle each vsir instruction when the src and dst register types don't match, for instance in `spirv_compiler_emit_alu_instruction()`.
So my impression now is that CMP, SLT, and SGE, are not so deserving of a normalization pass that only changes the `handler_idx` and appends a cast from bool to float.
But if we do, do we have a vsir `handler_idx` to represent such casts (which will have to end up as SpvOpSelect)?
This merge request was approved by Francisco Casas.
Minor: the commit subject for 1/6 is lowercase.
Giovanni Mascellani (@giomasce) commented about tests/hlsl/float-comparison.shader_test:
+% Check that -0.0f is not less than 0.0f +[pixel shader todo(sm<4)] +float a;
+float4 main() : sv_target +{
- return -0.0f < a;
Any particular reason for passing one operand as immediate and the other as uniform?
This merge request was approved by Giovanni Mascellani.
On Fri Mar 8 15:11:08 2024 +0000, Giovanni Mascellani wrote:
Any particular reason for passing one operand as immediate and the other as uniform?
At least one has to be uniform to ensure that this is not being constant folded and the purpose of the test seemed more clear letting -0.0f in the shader. Also that way I could check at native's output to confirm that it is indeed emitting the constant as `-0.0f`.
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)