[PATCH v5 0/7] 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. -- v5: 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 8591fe31..245393b0 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.c | 10 +++++----- libs/vkd3d-shader/hlsl.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 245393b0..7fb7e6b0 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -277,9 +277,9 @@ static bool type_is_single_component(const struct hlsl_type *type) * So, this function can be called several times in sequence to obtain all the path's indexes until * the component is finally reached. */ static unsigned int traverse_path_from_component_index(struct hlsl_ctx *ctx, - struct hlsl_type **type_ptr, unsigned int *index_ptr) + const struct hlsl_type **type_ptr, unsigned int *index_ptr) { - struct hlsl_type *type = *type_ptr; + const struct hlsl_type *type = *type_ptr; unsigned int index = *index_ptr; assert(!type_is_single_component(type)); @@ -341,13 +341,13 @@ static unsigned int traverse_path_from_component_index(struct hlsl_ctx *ctx, } } -struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type, +struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, const struct hlsl_type *type, unsigned int index) { while (!type_is_single_component(type)) traverse_path_from_component_index(ctx, &type, &index); - return type; + return (struct hlsl_type *) type; } static bool init_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_var *var, @@ -394,7 +394,7 @@ static bool init_deref_from_component_index(struct hlsl_ctx *ctx, struct hlsl_bl const struct vkd3d_shader_location *loc) { unsigned int path_len, path_index, deref_path_len, i; - struct hlsl_type *path_type; + const struct hlsl_type *path_type; struct hlsl_ir_constant *c; list_init(&block->instrs); diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index f237d6c4..724d157e 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -815,7 +815,7 @@ struct hlsl_type *hlsl_type_clone(struct hlsl_ctx *ctx, struct hlsl_type *old, unsigned int default_majority, unsigned int modifiers); unsigned int hlsl_type_component_count(const struct hlsl_type *type); unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type); -struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type, +struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, const struct hlsl_type *type, unsigned int index); bool hlsl_type_is_row_major(const struct hlsl_type *type); unsigned int hlsl_type_minor_size(const struct hlsl_type *type); -- 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 aacaa95c..ca5e9b64 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2073,6 +2073,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 7ada218c..5e07415b 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1839,6 +1839,26 @@ static void initialize_var_components(struct hlsl_ctx *ctx, struct list *instrs, } } +static bool type_has_object_components(struct hlsl_type *type, bool within_struct) +{ + if (type->type == HLSL_CLASS_OBJECT) + return !within_struct; + if (type->type == HLSL_CLASS_ARRAY) + return type_has_object_components(type->e.array.type, within_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) { @@ -1974,6 +1994,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 | 29 +++++++++++++++++++++++++++++ tests/hlsl-invalid.shader_test | 14 ++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 5e07415b..4c45e716 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1859,6 +1859,28 @@ static bool type_has_object_components(struct hlsl_type *type, bool within_struc 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) { @@ -2028,6 +2050,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..460fd78a 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 ca5e9b64..87145ddb 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1897,7 +1897,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); } @@ -1916,7 +1916,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); } @@ -1979,7 +1979,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, @@ -2081,7 +2081,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 Zebediah Figura. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42
From patch 2/7: ```diff -struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type, +struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, const struct hlsl_type *type, unsigned int index) { while (!type_is_single_component(type)) traverse_path_from_component_index(ctx, &type, &index);
- return type; + return (struct hlsl_type *) type; } ``` This ends up casting away const, which seems undesirable. Is that really unavoidable?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_12811
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
} }
+static bool type_has_object_components(struct hlsl_type *type, bool within_struct)
I am bit puzzled by the name `within_struct`. It seems to me that that variable is set when we are *not* inside a struct, isn't it? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_12974
On Thu Nov 3 13:31:28 2022 +0000, Giovanni Mascellani wrote:
I am bit puzzled by the name `within_struct`. It seems to me that that variable is set when we are *not* inside a struct, isn't it? The idea of that argument is to add the extra requirement for the object component to be within a struct inside the type.
So if you pass `true`, you are asking:
type has object components within struct ?
Maybe I should rename the flag to `must_be_in_struct` or create a different function `type_has_object_components_within_struct()` at the expense of some redundancy. What do you think? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_12981
On Thu Nov 3 14:05:23 2022 +0000, Francisco Casas wrote:
The idea of that argument is to add the extra requirement for the object component to be within a struct inside the type. So if you pass `true`, you are asking:
type has object components within struct ? Maybe I should rename the flag to `must_be_in_struct` or create a different function `type_has_object_components_within_struct()` at the expense of some redundancy. What do you think? Ok, right, it makes sense.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_12982
On Thu Nov 3 18:25:18 2022 +0000, Henri Verbeet wrote:
From patch 2/7:
```diff -struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx,
struct hlsl_type *type,
+struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, const struct hlsl_type *type, unsigned int index) { while (!type_is_single_component(type)) traverse_path_from_component_index(ctx, &type, &index);
- return type; + return (struct hlsl_type *) type; } ``` This ends up casting away const, which seems undesirable. Is that really unavoidable? Well, my memory of why I did this is fuzzy. I remember that I spent a lot of time adding `const` modifiers until I settled into this.
I think that the problem went along these lines: In C, a double pointer `type **` cannot be casted implicitly to `const type **` without a warning. e.g.: ```c int main() { int a = 2; int *p = &a; int **pp = &p; const int **cpp = pp; } ``` (there is an interesting SO thread here: https://stackoverflow.com/questions/5055655/double-pointer-const-correctness...) So, adding the const as the argument of `hlsl_type_get_component_type()` was more of a requirement to avoid this. I could have settled on doing: ```c struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type, unsigned int index) { cont struct hlsl_type *ctype = type; while (!type_is_single_component(type)) traverse_path_from_component_index(ctx, &ctype, &index); return (struct hlsl_type *) ctype; } ``` but adding the `const` in the signature of `hlsl_type_get_component_type()` makes it a little more correct. OTOH if I went full `const`, and I were to add `const` to the return type of `hlsl_type_get_component_type()` (which in principle seems to be the correct thing to do), then I would have to add it to the `struct hlsl_type *` variables in the callers: ```c [const] hlsl_type *type; type = hlsl_type_get_component_type(·); ``` But these same variables are passed to other functions, and we have a lot of functions that receive `struct hlsl_type *` instead of `const struct hlsl_type *`, so we would have to change those signatures too, and probably in the `struct hlsl_type *` members in structs like `struct hlsl_ir_var` too. Now, I must confess that I don't recall what was the motivation for this specific patch, besides performing a little step towards const correctness. I don't remember if we discussed this many moons ago... I guess I could remove it from the series if it just adds noise. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13030
On Thu Nov 3 18:25:18 2022 +0000, Francisco Casas wrote:
Well, my memory of why I did this is fuzzy. I remember that I spent a lot of time adding `const` modifiers until I settled into this. I think that the problem went along these lines: In C, a double pointer `type **` cannot be casted implicitly to `const type **` without a warning. e.g.: ```c int main() { int a = 2; int *p = &a; int **pp = &p; const int **cpp = pp; } ``` (there is an interesting SO thread here: https://stackoverflow.com/questions/5055655/double-pointer-const-correctness...) So, adding the const as the argument of `hlsl_type_get_component_type()` was more of a requirement to avoid this. I could have settled on doing: ```c struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type, unsigned int index) { cont struct hlsl_type *ctype = type; while (!type_is_single_component(type)) traverse_path_from_component_index(ctx, &ctype, &index); return (struct hlsl_type *) ctype; } ``` but adding the `const` in the signature of `hlsl_type_get_component_type()` makes it a little more correct. OTOH if I went full `const`, and I were to add `const` to the return type of `hlsl_type_get_component_type()` (which in principle seems to be the correct thing to do), then I would have to add it to the `struct hlsl_type *` variables in the callers: ```c [const] hlsl_type *type; type = hlsl_type_get_component_type(·); ``` But these same variables are passed to other functions, and we have a lot of functions that receive `struct hlsl_type *` instead of `const struct hlsl_type *`, so we would have to change those signatures too, and probably in the `struct hlsl_type *` members in structs like `struct hlsl_ir_var` too. Now, I must confess that I don't recall what was the motivation for this specific patch, besides performing a little step towards const correctness. I don't remember if we discussed this many moons ago... I guess I could remove it from the series if it just adds noise. Propagating const to more functions is probably desirable in general. The problem is that it may be a losing battle, given "bytecode_offset". I'm not sure that avoiding that would be an improvement, though...
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13043
OTOH if I went full `const`, and I were to add `const` to the return type of `hlsl_type_get_component_type()` (which in principle seems to be the correct thing to do), then I would have to add it to the `struct hlsl_type *` variables in the callers:
```c [const] hlsl_type *type;
type = hlsl_type_get_component_type(·); ```
But these same variables are passed to other functions, and we have a lot of functions that receive `struct hlsl_type *` instead of `const struct hlsl_type *`, so we would have to change those signatures too, and probably in the `struct hlsl_type *` members in structs like `struct hlsl_ir_var` too.
We went through making a bunch of things const in wined3d some years ago now, and you pretty much have to start from the leaf functions and work your way up from there. You'll likely still end up with a number of cases where it's just not worth it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13066
On Thu Nov 3 19:10:23 2022 +0000, Zebediah Figura wrote:
Propagating const to more functions is probably desirable in general. The problem is that it may be a losing battle, given "bytecode_offset". I'm not sure that avoiding that would be an improvement, though... Personally I like to have everything as `const` as possible, but (in general) not at the expense of casting away `const`.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13165
On Fri Nov 4 13:45:37 2022 +0000, Giovanni Mascellani wrote:
Personally I like to have everything as `const` as possible, but (in general) not at the expense of casting away `const`. In the spirit of e.g. strchr(), I don't mind the patch as-is. I'll defer to Henri's preference, though.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13185
Dropping 'const' patch. And renaming `within_struct` parameter to `must_be_in_struct` ... -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/42#note_13414
participants (5)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)