[PATCH v8 0/6] MR42: vkd3d-shader/hlsl: Some fixes.
A mix of a miscellaneous fixes: * Fixes to failed asserts I have stumbled upon when implementing other features. * Checks required for properly supporting object components. * A couple of code improvements. -- v8: vkd3d-shader/hlsl: Use reg_size as component count when allocating a single register. vkd3d-shader/hlsl: Use the base type of the array elements in write_sm1_type(). vkd3d-shader/hlsl: Validate that statics don't contain both resources and numerics. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
From: Francisco Casas <fcasas(a)codeweavers.com> --- --- libs/vkd3d-shader/hlsl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 1f35b1ac..4edad822 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -885,7 +885,10 @@ struct hlsl_ir_store *hlsl_new_store_index(struct hlsl_ctx *ctx, const struct hl init_node(&store->node, HLSL_IR_STORE, NULL, loc); if (!init_deref(ctx, &store->lhs, lhs->var, lhs->path_len + !!idx)) + { + vkd3d_free(store); return NULL; + } for (i = 0; i < lhs->path_len; ++i) hlsl_src_from_node(&store->lhs.path[i], lhs->path[i].node); if (idx) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 6e650db0..e6bbce3f 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2103,6 +2103,9 @@ static void allocate_const_registers(struct hlsl_ctx *ctx, struct hlsl_ir_functi { if (var->is_uniform && var->last_read) { + if (var->data_type->reg_size == 0) + continue; + if (var->data_type->reg_size > 4) var->reg = allocate_range(ctx, &liveness, 1, UINT_MAX, var->data_type->reg_size); else -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
From: Francisco Casas <fcasas(a)codeweavers.com> It is worth noting that these checks should also be included for declarations inside cbuffers, once they are implemented. --- libs/vkd3d-shader/hlsl.y | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 00842195..7c349c75 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1881,6 +1881,26 @@ static void initialize_var_components(struct hlsl_ctx *ctx, struct list *instrs, } } +static bool type_has_object_components(struct hlsl_type *type, bool must_be_in_struct) +{ + if (type->type == HLSL_CLASS_OBJECT) + return !must_be_in_struct; + if (type->type == HLSL_CLASS_ARRAY) + return type_has_object_components(type->e.array.type, must_be_in_struct); + + if (type->type == HLSL_CLASS_STRUCT) + { + unsigned int i; + + for (i = 0; i < type->e.record.field_count; ++i) + { + if (type_has_object_components(type->e.record.fields[i].type, false)) + return true; + } + } + return false; +} + static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_type, DWORD modifiers, struct list *var_list) { @@ -2016,6 +2036,13 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t if (!(modifiers & HLSL_STORAGE_STATIC)) var->modifiers |= HLSL_STORAGE_UNIFORM; + if (ctx->profile->major_version < 5 && (var->modifiers & HLSL_STORAGE_UNIFORM) && + type_has_object_components(var->data_type, true)) + { + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Target profile doesn't support objects as struct members in uniform variables.\n"); + } + if ((func = hlsl_get_func_decl(ctx, var->name))) { hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 27 +++++++++++++++++++++++++++ tests/hlsl-invalid.shader_test | 14 ++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 7c349c75..3e2a0ed0 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1901,6 +1901,26 @@ static bool type_has_object_components(struct hlsl_type *type, bool must_be_in_s return false; } +static bool type_has_numeric_components(struct hlsl_type *type) +{ + if (type->type <= HLSL_CLASS_LAST_NUMERIC) + return true; + if (type->type == HLSL_CLASS_ARRAY) + return type_has_numeric_components(type->e.array.type); + + if (type->type == HLSL_CLASS_STRUCT) + { + unsigned int i; + + for (i = 0; i < type->e.record.field_count; ++i) + { + if (type_has_numeric_components(type->e.record.fields[i].type)) + return true; + } + } + return false; +} + static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_type, DWORD modifiers, struct list *var_list) { @@ -2070,6 +2090,13 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t "Semantics are not allowed on local variables."); } + if ((var->modifiers & HLSL_STORAGE_STATIC) && type_has_numeric_components(var->data_type) + && type_has_object_components(var->data_type, false)) + { + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Static variables cannot have both numeric and resource components."); + } + if ((type->modifiers & HLSL_MODIFIER_CONST) && !v->initializer.args_count && !(modifiers & (HLSL_STORAGE_STATIC | HLSL_STORAGE_UNIFORM))) { diff --git a/tests/hlsl-invalid.shader_test b/tests/hlsl-invalid.shader_test index a7364b1e..1b39d7f0 100644 --- a/tests/hlsl-invalid.shader_test +++ b/tests/hlsl-invalid.shader_test @@ -250,3 +250,17 @@ float4 main() : sv_target a.a = 1; return a.a; } + +[pixel shader fail] +struct apple +{ + sampler sam; + float4 aa; +}; + +static struct apple a; + +float4 main() : sv_target +{ + return 1.0; +} -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_sm1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl_sm1.c b/libs/vkd3d-shader/hlsl_sm1.c index 2219ef56..ba22925e 100644 --- a/libs/vkd3d-shader/hlsl_sm1.c +++ b/libs/vkd3d-shader/hlsl_sm1.c @@ -272,7 +272,7 @@ static void write_sm1_type(struct vkd3d_bytecode_buffer *buffer, struct hlsl_typ } } - type->bytecode_offset = put_u32(buffer, vkd3d_make_u32(sm1_class(type), sm1_base_type(type))); + type->bytecode_offset = put_u32(buffer, vkd3d_make_u32(sm1_class(type), sm1_base_type(array_type))); put_u32(buffer, vkd3d_make_u32(type->dimy, type->dimx)); put_u32(buffer, vkd3d_make_u32(array_size, field_count)); put_u32(buffer, fields_offset); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
From: Francisco Casas <fcasas(a)codeweavers.com> Otherwise, for instance, the added test results in: debug_hlsl_writemask: Assertion `!(writemask & ~VKD3DSP_WRITEMASK_ALL)' failed. Which happens in allocate_variable_temp_register() when the variable's type reg_size is <= 4 but its component count is larger, which may happen if it contains objects. --- v2: - Replacing "dimx" with "reg_size" on all other places where allocate_register() is called, since it was only there for historical reasons. --- libs/vkd3d-shader/hlsl_codegen.c | 8 ++++---- tests/object-references.shader_test | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index e6bbce3f..e55c6082 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1927,7 +1927,7 @@ static void allocate_variable_temp_register(struct hlsl_ctx *ctx, struct hlsl_ir var->last_read, var->data_type->reg_size); else var->reg = allocate_register(ctx, liveness, var->first_write, - var->last_read, hlsl_type_component_count(var->data_type)); + var->last_read, var->data_type->reg_size); TRACE("Allocated %s to %s (liveness %u-%u).\n", var->name, debug_register('r', var->reg, var->data_type), var->first_write, var->last_read); } @@ -1946,7 +1946,7 @@ static void allocate_temp_registers_recurse(struct hlsl_ctx *ctx, struct hlsl_bl instr->last_read, instr->data_type->reg_size); else instr->reg = allocate_register(ctx, liveness, instr->index, - instr->last_read, instr->data_type->dimx); + instr->last_read, instr->data_type->reg_size); TRACE("Allocated anonymous expression @%u to %s (liveness %u-%u).\n", instr->index, debug_register('r', instr->reg, instr->data_type), instr->index, instr->last_read); } @@ -2009,7 +2009,7 @@ static void allocate_const_registers_recurse(struct hlsl_ctx *ctx, struct hlsl_b if (reg_size > 4) constant->reg = allocate_range(ctx, liveness, 1, UINT_MAX, reg_size); else - constant->reg = allocate_register(ctx, liveness, 1, UINT_MAX, type->dimx); + constant->reg = allocate_register(ctx, liveness, 1, UINT_MAX, reg_size); TRACE("Allocated constant @%u to %s.\n", instr->index, debug_register('c', constant->reg, type)); if (!hlsl_array_reserve(ctx, (void **)&defs->values, &defs->size, @@ -2111,7 +2111,7 @@ static void allocate_const_registers(struct hlsl_ctx *ctx, struct hlsl_ir_functi else { var->reg = allocate_register(ctx, &liveness, 1, UINT_MAX, 4); - var->reg.writemask = (1u << var->data_type->dimx) - 1; + var->reg.writemask = (1u << var->data_type->reg_size) - 1; } TRACE("Allocated %s to %s.\n", var->name, debug_register('c', var->reg, var->data_type)); } diff --git a/tests/object-references.shader_test b/tests/object-references.shader_test index 6cff351a..1276dd36 100644 --- a/tests/object-references.shader_test +++ b/tests/object-references.shader_test @@ -118,3 +118,28 @@ float4 main() : sv_target { return tex[n].Load(0); } + + +[pixel shader] +Texture2D tex; +uniform float f; + +struct apple +{ + Texture2D tex1; + Texture2D tex2; + float3 aa; +}; + +float4 main() : sv_target +{ + struct apple a = {tex, tex, 1.0, 2.0, 3.0}; + + a.aa += f; + return a.aa.xyzx; +} + +[test] +uniform 0 float 10.0 +todo draw quad +probe (0, 0) rgba (11.0, 12.0, 13.0, 11.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
tests/object-references.shader_test is failing for me. Specifically: ``` shader_runner:684:Section [pixel shader], line 123: Test failed: Got unexpected hr 0x80004001. ``` That's probably a consequence of Zeb's merge request 43 ("hlsl: Compute shaders."). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13729
On Tue Nov 8 17:49:30 2022 +0000, Henri Verbeet wrote:
tests/object-references.shader_test is failing for me. Specifically: ``` shader_runner:684:Section [pixel shader], line 123: Test failed: Got unexpected hr 0x80004001. ``` That's probably a consequence of Zeb's merge request 43 ("hlsl: Compute shaders."). I see, rebasing !42 over !43 results in
E5017: Aborting due to not yet implemented feature: Object copy.
I will investigate further. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13740
On Tue Nov 8 17:49:29 2022 +0000, Francisco Casas wrote:
I see, rebasing !42 over !43 results in ``` E5017: Aborting due to not yet implemented feature: Object copy. ``` I will investigate further. The problem is that the test leaves behind an hlsl_ir_load of an object type (instruction 3):
``` trace:hlsl_dump_function 2: float | f trace:hlsl_dump_function 3: Texture2D<float4> | tex trace:hlsl_dump_function 4: float | 1.00000000e+00 trace:hlsl_dump_function 5: float | 2.00000000e+00 trace:hlsl_dump_function 6: float | 3.00000000e+00 trace:hlsl_dump_function 7: uint | 0 trace:hlsl_dump_function 8: | = (a[@7]. @3) trace:hlsl_dump_function 9: uint | 0 trace:hlsl_dump_function 10: | = (a[@9]. @3) trace:hlsl_dump_function 11: uint | 0 trace:hlsl_dump_function 12: | = (a[@11].x @4) trace:hlsl_dump_function 13: uint | 1 trace:hlsl_dump_function 14: | = (a[@13].x @5) trace:hlsl_dump_function 15: uint | 2 trace:hlsl_dump_function 16: | = (a[@15].x @6) trace:hlsl_dump_function 17: uint | 0 trace:hlsl_dump_function 18: float3 | a[@17] trace:hlsl_dump_function 19: float3 | @2.xxx trace:hlsl_dump_function 20: float3 | + (@18 @19 ) trace:hlsl_dump_function 21: float4 | @20.xyzx trace:hlsl_dump_function 22: | return trace:hlsl_dump_function 23: | = (<output-sv_target0> @21) ``` These kinds of hlsl_ir_loads are necessary so that, when there are object loads and stores, copy propagation can make them point their derefs to the static variables, even if they go through several assignments. However, these types of loads should not reach the sm1/sm4 translations to bytecode because either: a) If the user made a non-constant dereference to an object type that cannot be solved statically, that is a compilation error and should be caught by the validate_static_object_references pass. b) If it is a constant dereference, copy propagation should connect the uses of these objects (in resource loads and stores) directly with the uniform variable, and leave the temps without use, so that they are removed by dce. The problem is that this second case is currently not handled properly because, as Zeb has pointed out in my broaded resources support MR, we currently only detect usage with variable level granularity, not component level granularity. So dce is not cleaning these instructions. Naturally, the proper solution is detecting usage with a component level granularity. That means making the fields first_write and last_read from hlsl_ir_var component-wise arrays. I will work on that for my broaden resources support v2 patches. As for what why the test was passing before the rebase, it is because the test block is written as: ``` [test] uniform 0 float 10.0 todo draw quad probe (0, 0) rgba (11.0, 12.0, 13.0, 11.0) ``` and apparently, after Zebs' patches the `todo` should also be added to `probe`, so I am doing that. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13839
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)