[PATCH v11 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. -- v11: 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. vkd3d-shader/hlsl: Validate that extern structs don't contain objects SM < 5. vkd3d-shader/hlsl: Don't allocate object types as constant registers. vkd3d-shader/hlsl: Properly free new store node memory if init_deref() fails. 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 eddbf2c8..8aa289ac 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 27af49cf..18c26771 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 f77b0b61..0e97d4d1 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1887,6 +1887,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) { @@ -2022,6 +2042,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 0e97d4d1..6461ade5 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1907,6 +1907,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) { @@ -2076,6 +2096,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 18c26771..58d44c4d 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 f3fda472..a33bd14a 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 todo] +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 +todo probe (0, 0) rgba (11.0, 12.0, 13.0, 11.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
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.
Yeah, I think so. The other option is that we actually split temporary variables into smaller pieces, which I think had been proposed at some point, but thinking about it further, that doesn't really have much of an advantage versus doing things component-wise. In particular you'd have to either completely split up vectors, or forgo doing per-component passes (DCE, copy-prop), and if you have to implement per-component passes anyway then there's not really a difference between that and doing them for larger variables. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13868
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)