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.
-- v3: vkd3d-shader/hlsl: Use reg_size as component count when allocating a single register. vkd3d-shader/hlsl: For arrays, return components' base type in sm1_base_type(). vkd3d-shader/hlsl: Validate that objects are not components of structs in shader models < 5. tests: Add SM5 requirement to tests with object components. vkd3d-shader/hlsl: Don't allocate object types as constant registers. vkd3d-shader/hlsl: Make hlsl_type_get_component_type() type argument const.
From: Francisco Casas fcasas@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)
From: Francisco Casas fcasas@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);
From: Francisco Casas fcasas@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
From: Francisco Casas fcasas@codeweavers.com
--- .../cast-componentwise-compatible.shader_test | 132 ++++++++--------- tests/cast-componentwise-equal.shader_test | 134 +++++++++--------- 2 files changed, 137 insertions(+), 129 deletions(-)
diff --git a/tests/cast-componentwise-compatible.shader_test b/tests/cast-componentwise-compatible.shader_test index da55628b..ca7ea05e 100644 --- a/tests/cast-componentwise-compatible.shader_test +++ b/tests/cast-componentwise-compatible.shader_test @@ -59,70 +59,6 @@ draw quad probe all rgba (1.0, 2.0, 3.0, 4.0)
-[pixel shader] -Texture2D tex; - -struct apple -{ - int2 aa; - Texture2D bb; - float cc; -}; - -struct banana -{ - float aa[2]; - Texture2D bb; - int cc; -}; - -float4 main() : sv_target -{ - struct apple a = {1, 2, tex, 7.2}; - struct banana b; - - b = (struct banana) a; - return b.cc; -} - -[test] -draw quad -probe all rgba (7.0, 7.0, 7.0, 7.0) - - -[pixel shader] -Texture2D tex; - -struct apple -{ - int2 aa; - Texture2D bb; - float cc; - float4 dd; - Texture2D ee; -}; - -struct banana -{ - float aa[2]; - Texture2D bb; - int cc; -}; - -float4 main() : sv_target -{ - struct apple a = {1, 2, tex, 3, 4, 5, 6, 7, tex}; - struct banana b; - - b = (struct banana) a; - return b.cc; -} - -[test] -draw quad -probe all rgba (3.0, 3.0, 3.0, 3.0) - - [pixel shader fail] struct apple { @@ -582,3 +518,71 @@ float4 main() : sv_target [test] draw quad probe all rgba (71.0, 72.0, 73.0, 0.0) + + +[require] +shader model >= 5.0 + + +[pixel shader] +Texture2D tex; + +struct apple +{ + int2 aa; + Texture2D bb; + float cc; +}; + +struct banana +{ + float aa[2]; + Texture2D bb; + int cc; +}; + +float4 main() : sv_target +{ + struct apple a = {1, 2, tex, 7.2}; + struct banana b; + + b = (struct banana) a; + return b.cc; +} + +[test] +draw quad +probe all rgba (7.0, 7.0, 7.0, 7.0) + + +[pixel shader] +Texture2D tex; + +struct apple +{ + int2 aa; + Texture2D bb; + float cc; + float4 dd; + Texture2D ee; +}; + +struct banana +{ + float aa[2]; + Texture2D bb; + int cc; +}; + +float4 main() : sv_target +{ + struct apple a = {1, 2, tex, 3, 4, 5, 6, 7, tex}; + struct banana b; + + b = (struct banana) a; + return b.cc; +} + +[test] +draw quad +probe all rgba (3.0, 3.0, 3.0, 3.0) diff --git a/tests/cast-componentwise-equal.shader_test b/tests/cast-componentwise-equal.shader_test index 4c85b9ed..34f1abbb 100644 --- a/tests/cast-componentwise-equal.shader_test +++ b/tests/cast-componentwise-equal.shader_test @@ -93,71 +93,6 @@ draw quad probe all rgba (5.0, 6.0, 7.0, 8.0)
-[pixel shader] -Texture2D tex; - -struct apple -{ - int2 aa; - Texture2D bb; - float cc; -}; - -struct banana -{ - int aa[2]; - Texture2D bb; - float cc; -}; - -float4 main() : sv_target -{ - struct apple a = {1, 2, tex, 4}; - struct banana b; - - b = a; - return b.cc; -} - -[test] -draw quad -probe all rgba (4.0, 4.0, 4.0, 4.0) - - -[pixel shader] -Texture2D tex; - -struct apple -{ - int2 aa; - Texture2D bb; - float cc; -}; - -struct banana -{ - int aa[2]; - Texture2D bb; - float cc; -}; - -float4 fun(struct banana b) -{ - return b.cc; -} - -float4 main() : sv_target -{ - struct apple a = {1, 2, tex, 5}; - - return fun(a); -} - -[test] -todo draw quad -todo probe all rgba (5.0, 5.0, 5.0, 5.0) - - [pixel shader] struct apple { @@ -314,3 +249,72 @@ float4 main() : SV_TARGET
return 0; } + + +[require] +shader model >= 5.0 + + +[pixel shader] +Texture2D tex; + +struct apple +{ + int2 aa; + Texture2D bb; + float cc; +}; + +struct banana +{ + int aa[2]; + Texture2D bb; + float cc; +}; + +float4 main() : sv_target +{ + struct apple a = {1, 2, tex, 4}; + struct banana b; + + b = a; + return b.cc; +} + +[test] +draw quad +probe all rgba (4.0, 4.0, 4.0, 4.0) + + +[pixel shader] +Texture2D tex; + +struct apple +{ + int2 aa; + Texture2D bb; + float cc; +}; + +struct banana +{ + int aa[2]; + Texture2D bb; + float cc; +}; + +float4 fun(struct banana b) +{ + return b.cc; +} + +float4 main() : sv_target +{ + struct apple a = {1, 2, tex, 5}; + + return fun(a); +} + +[test] +todo draw quad +todo probe all rgba (5.0, 5.0, 5.0, 5.0)
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl.c | 29 ++++++++++++++++++++++++++++- libs/vkd3d-shader/hlsl.h | 2 +- libs/vkd3d-shader/hlsl.y | 4 ++-- tests/object-references.shader_test | 2 +- 4 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 7fb7e6b0..07829c29 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -488,8 +488,28 @@ struct hlsl_type *hlsl_new_array_type(struct hlsl_ctx *ctx, struct hlsl_type *ba return type; }
+static bool type_has_object_components(struct hlsl_type *type) +{ + if (type->type == HLSL_CLASS_OBJECT) + return true; + if (type->type == HLSL_CLASS_ARRAY) + return type_has_object_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_object_components(type->e.record.fields[i].type)) + return true; + } + } + return false; +} + struct hlsl_type *hlsl_new_struct_type(struct hlsl_ctx *ctx, const char *name, - struct hlsl_struct_field *fields, size_t field_count) + struct hlsl_struct_field *fields, size_t field_count, const struct vkd3d_shader_location *loc) { struct hlsl_type *type;
@@ -501,6 +521,13 @@ struct hlsl_type *hlsl_new_struct_type(struct hlsl_ctx *ctx, const char *name, type->dimy = 1; type->e.record.fields = fields; type->e.record.field_count = field_count; + + if (ctx->profile->major_version < 5 && type_has_object_components(type)) + { + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Target profile doesn't support objects as struct members.\n"); + } + hlsl_type_calculate_reg_size(ctx, type);
list_add_tail(&ctx->types, &type->entry); diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 724d157e..a832c6a6 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -782,7 +782,7 @@ struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, struct vkd3d_shader_loc struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx, const struct hlsl_resource_load_params *params, const struct vkd3d_shader_location *loc); struct hlsl_type *hlsl_new_struct_type(struct hlsl_ctx *ctx, const char *name, - struct hlsl_struct_field *fields, size_t field_count); + struct hlsl_struct_field *fields, size_t field_count, const struct vkd3d_shader_location *loc); struct hlsl_ir_swizzle *hlsl_new_swizzle(struct hlsl_ctx *ctx, DWORD s, unsigned int components, struct hlsl_ir_node *val, const struct vkd3d_shader_location *loc); struct hlsl_ir_var *hlsl_new_synthetic_var(struct hlsl_ctx *ctx, const char *template, diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 7ada218c..a84e37ba 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -3307,7 +3307,7 @@ named_struct_spec: { bool ret;
- $$ = hlsl_new_struct_type(ctx, $2, $4.fields, $4.count); + $$ = hlsl_new_struct_type(ctx, $2, $4.fields, $4.count, &@$);
if (hlsl_get_var(ctx->cur_scope, $2)) { @@ -3326,7 +3326,7 @@ named_struct_spec: unnamed_struct_spec: KW_STRUCT '{' fields_list '}' { - $$ = hlsl_new_struct_type(ctx, NULL, $3.fields, $3.count); + $$ = hlsl_new_struct_type(ctx, NULL, $3.fields, $3.count, &@$); }
any_identifier: diff --git a/tests/object-references.shader_test b/tests/object-references.shader_test index 6cff351a..56d58804 100644 --- a/tests/object-references.shader_test +++ b/tests/object-references.shader_test @@ -1,5 +1,5 @@ [require] -shader model >= 4.0 +shader model >= 5.0
[texture 0] size (2, 2)
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl_sm1.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl_sm1.c b/libs/vkd3d-shader/hlsl_sm1.c index 2219ef56..229f6895 100644 --- a/libs/vkd3d-shader/hlsl_sm1.c +++ b/libs/vkd3d-shader/hlsl_sm1.c @@ -165,6 +165,9 @@ static D3DXPARAMETER_CLASS sm1_class(const struct hlsl_type *type)
static D3DXPARAMETER_TYPE sm1_base_type(const struct hlsl_type *type) { + if (type->type == HLSL_CLASS_ARRAY) + return sm1_base_type(type->e.array.type); + switch (type->base_type) { case HLSL_TYPE_BOOL:
From: Francisco Casas fcasas@codeweavers.com
Otherwise the added test results in:
debug_hlsl_writemask: Assertion `!(writemask & ~VKD3DSP_WRITEMASK_ALL)' failed.
Which happens when the variable's type reg_size is <= 4 but its component count is larger, which may happen if it contains objects.
---
In other places where allocate_register() is called, dimx is used instead of reg_size. I think this doesn't cause an error even when structs contain objects (which get the dimx from their format type), thanks to the split_array_copies and split_struct_copies passes, but it makes me wonder why not simply use reg_size in those cases too. --- libs/vkd3d-shader/hlsl_codegen.c | 2 +- tests/object-references.shader_test | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index ca5e9b64..dc165f7e 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); } diff --git a/tests/object-references.shader_test b/tests/object-references.shader_test index 56d58804..1ceada01 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)
All of the tests pass for me with the native compiler, which implies that the changes in patch 5/7 are not correct as-is (and, somewhat by extension, neither are the changes in 4/7.)
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_sm1.c:
static D3DXPARAMETER_TYPE sm1_base_type(const struct hlsl_type *type) {
- if (type->type == HLSL_CLASS_ARRAY)
return sm1_base_type(type->e.array.type);
sm1_class() does this too, but we should probably just pass "array_type" from write_sm1_type().
In other places where allocate_register() is called, dimx is used instead of reg_size. I think this doesn't cause an error even when structs contain objects (which get the dimx from their format type), thanks to the split_array_copies and split_struct_copies passes, but it makes me wonder why not simply use reg_size in those cases too.
As described off-list, the answer is mostly "historical reasons". When reg_size was introduced (Wine commit 5f18f9f75ac4!) it counted whole registers, mostly because the very earliest register allocation code I wrote was geared towards sm1, and sm4 support was only added later. And non-numeric registers simply weren't considered at that point.
On Tue Nov 1 13:49:05 2022 +0000, Zebediah Figura wrote:
All of the tests pass for me with the native compiler, which implies that the changes in patch 5/7 are not correct as-is (and, somewhat by extension, neither are the changes in 4/7.)
(!)
I see. So I was mistaken. It seems that objects can actually be components of structs in SM < 5 if they are used as local variables, as in the tests.
Probably I got confused, because, for instance, this shader fails in ps_4_0: ```c struct apple { int2 aa; Texture2D bb; float cc; };
struct apple a; // Uniform variable of type that has object components!
float4 main() : sv_target { return float4(0, 1, 2, 3); }
``` and the error message is a little more general than it should be: ``` error X3090: ps_4_0 does not allow textures or samplers to be members of compound types ```
I will check more in detail which storage classes are allowed to have object members and which don't.
On Tue Nov 1 13:50:04 2022 +0000, Zebediah Figura wrote:
In other places where allocate_register() is called, dimx is used
instead of reg_size. I think this doesn't cause an error even when structs contain objects (which get the dimx from their format type), thanks to the split_array_copies and split_struct_copies passes, but it makes me wonder why not simply use reg_size in those cases too. As described off-list, the answer is mostly "historical reasons". When reg_size was introduced (Wine commit 5f18f9f75ac4!) it counted whole registers, mostly because the very earliest register allocation code I wrote was geared towards sm1, and sm4 support was only added later. And non-numeric registers simply weren't considered at that point.
Ok, then I will change all other uses of `dimx` for allocating a register to `reg_size` in v2.
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Zebediah Figura (＠zfigura)