[PATCH vkd3d 1/8] vkd3d-shader/hlsl: Detect missing loads on rhs when splitting copies of non-numeric types.
From: Francisco Casas <fcasas(a)codeweavers.com> Otherwise we can get failed assertions: assert(node->type == HLSL_IR_LOAD); because broadcasts to these types are not implemented yet. Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index e945b94d..f7396a96 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -637,6 +637,12 @@ static bool split_array_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, element_type = type->e.array.type; element_size = hlsl_type_get_array_element_reg_size(element_type); + if (rhs->type != HLSL_IR_LOAD) + { + hlsl_fixme(ctx, &instr->loc, "Array store rhs is not HLSL_IR_LOAD. Broadcast may be missing."); + return false; + } + for (i = 0; i < type->e.array.elements_count; ++i) { if (!split_copy(ctx, store, hlsl_ir_load(rhs), i * element_size, element_type)) @@ -667,6 +673,12 @@ static bool split_struct_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr if (type->type != HLSL_CLASS_STRUCT) return false; + if (rhs->type != HLSL_IR_LOAD) + { + hlsl_fixme(ctx, &instr->loc, "Struct store rhs is not HLSL_IR_LOAD. Broadcast may be missing."); + return false; + } + LIST_FOR_EACH_ENTRY(field, type->e.elements, struct hlsl_struct_field, entry) { if (!split_copy(ctx, store, hlsl_ir_load(rhs), field->reg_offset, field->type)) -- 2.36.0
From: Francisco Casas <fcasas(a)codeweavers.com> Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- Makefile.am | 1 + tests/cast-broadcast.shader_test | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 tests/cast-broadcast.shader_test diff --git a/Makefile.am b/Makefile.am index 26aa3f84..fa3c2a6d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -56,6 +56,7 @@ vkd3d_shader_tests = \ tests/arithmetic-int.shader_test \ tests/arithmetic-uint.shader_test \ tests/bitwise.shader_test \ + tests/cast-broadcast.shader_test \ tests/cast-to-float.shader_test \ tests/cast-to-half.shader_test \ tests/cast-to-int.shader_test \ diff --git a/tests/cast-broadcast.shader_test b/tests/cast-broadcast.shader_test new file mode 100644 index 00000000..02d14c0b --- /dev/null +++ b/tests/cast-broadcast.shader_test @@ -0,0 +1,24 @@ +[pixel shader] + +struct foo +{ + float3 aa; + float4 bb; +}; + +struct bar +{ + struct foo aa; + int2 bb; + int4 cc[8]; +}; + +float4 main() : SV_TARGET +{ + struct bar p = (struct bar)42; + return p.aa.bb + p.cc[5]; +} + +[test] +todo draw quad +todo probe all rgba (84.0, 84.0, 84.0, 84.0) -- 2.36.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- As a follow-up, adding a test for scalar-to-array casts would also be helpful.
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
From: Francisco Casas <fcasas(a)codeweavers.com> Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- Makefile.am | 1 + ...hlsl-initializer-implicit-array.shader_test | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 tests/hlsl-initializer-implicit-array.shader_test diff --git a/Makefile.am b/Makefile.am index fa3c2a6d..2742e77c 100644 --- a/Makefile.am +++ b/Makefile.am @@ -77,6 +77,7 @@ vkd3d_shader_tests = \ tests/hlsl-gather.shader_test \ tests/hlsl-initializer-flatten.shader_test \ tests/hlsl-initializer-invalid-arg-count.shader_test \ + tests/hlsl-initializer-implicit-array.shader_test \ tests/hlsl-initializer-local-array.shader_test \ tests/hlsl-initializer-nested.shader_test \ tests/hlsl-initializer-numeric.shader_test \ diff --git a/tests/hlsl-initializer-implicit-array.shader_test b/tests/hlsl-initializer-implicit-array.shader_test new file mode 100644 index 00000000..d2b94da4 --- /dev/null +++ b/tests/hlsl-initializer-implicit-array.shader_test @@ -0,0 +1,18 @@ +[pixel shader] +float4 main() : SV_TARGET +{ + float4 arr[] = {10, 20, 30, 40, 50, 60, 70, 80}; + return arr[1]; +} + +[test] +todo draw quad +todo probe all rgba (50, 60, 70, 80) + + +[pixel shader fail] +float4 main() : SV_TARGET +{ + float4 arr[] = {10, 20, 30, 40, 50, 60, 70}; + return arr[0]; +} -- 2.36.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
From: Francisco Casas <fcasas(a)codeweavers.com> HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT is used as a temporal value for elements_count for implicit size arrays. It is replaced by the correct value after parsing the initializer. Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.c | 13 ++++- libs/vkd3d-shader/hlsl.h | 2 + libs/vkd3d-shader/hlsl.y | 50 ++++++++++++++++++- libs/vkd3d-shader/hlsl_sm1.c | 1 + libs/vkd3d-shader/hlsl_sm4.c | 1 + ...lsl-initializer-implicit-array.shader_test | 4 +- 6 files changed, 67 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 7239b183..1d272866 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -126,6 +126,7 @@ static bool hlsl_type_is_row_major(const struct hlsl_type *type) static unsigned int get_array_size(const struct hlsl_type *type) { + assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); if (type->type == HLSL_CLASS_ARRAY) return get_array_size(type->e.array.type) * type->e.array.elements_count; return 1; @@ -166,6 +167,7 @@ static void hlsl_type_calculate_reg_size(struct hlsl_ctx *ctx, struct hlsl_type unsigned int element_size = type->e.array.type->reg_size; assert(element_size); + assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); if (is_sm4) type->reg_size = (type->e.array.elements_count - 1) * align(element_size, 4) + element_size; else @@ -328,6 +330,9 @@ struct hlsl_type *hlsl_new_array_type(struct hlsl_ctx *ctx, struct hlsl_type *ba list_add_tail(&ctx->types, &type->entry); + if (type->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + TRACE("Implicit element count.\n"); + return type; } @@ -410,6 +415,7 @@ unsigned int hlsl_type_component_count(struct hlsl_type *type) } if (type->type == HLSL_CLASS_ARRAY) { + assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); return hlsl_type_component_count(type->e.array.type) * type->e.array.elements_count; } if (type->type != HLSL_CLASS_STRUCT) @@ -1023,7 +1029,12 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru } for (t = type; t->type == HLSL_CLASS_ARRAY; t = t->e.array.type) - vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count); + { + if (t->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + vkd3d_string_buffer_printf(string, "[<undefined>]"); + else + vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count); + } return string; } diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 28b2ff1b..94597999 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -227,6 +227,8 @@ struct hlsl_src #define HLSL_MODIFIERS_MAJORITY_MASK (HLSL_MODIFIER_ROW_MAJOR | HLSL_MODIFIER_COLUMN_MAJOR) +#define HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 0xffffffff + struct hlsl_reg_reservation { char type; diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 905dbfc5..e7fe74d8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t type = basic_type; for (i = 0; i < v->arrays.count; ++i) + { + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + unsigned int size = initializer_size(&v->initializer); + unsigned int elem_components = hlsl_type_component_count(type); + + assert(v->initializer.args_count); + + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components; + + if (size % elem_components != 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Cannot initialize implicit array with %u components, expected a multiple of %u.", + size, elem_components); + free_parse_initializer(&v->initializer); + v->initializer.args_count = 0; + } + } type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); + } vkd3d_free(v->arrays.sizes); if (type->type != HLSL_CLASS_MATRIX) @@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %token <name> TYPE_IDENTIFIER %type <arrays> arrays +%type <arrays> implicit_arrays %type <assign_op> assign_op @@ -3108,7 +3129,7 @@ variables_def: } variable_decl: - any_identifier arrays colon_attribute + any_identifier implicit_arrays colon_attribute { $$ = hlsl_alloc(ctx, sizeof(*$$)); $$->loc = @1; @@ -3137,6 +3158,15 @@ state_block: variable_def: variable_decl + { + if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Implicit array requires initializer."); + free_parse_variable_def($$); + YYABORT; + } + } | variable_decl '=' complex_initializer { $$ = $1; @@ -3188,6 +3218,24 @@ arrays: $$.sizes[$$.count++] = size; } +implicit_arrays: + arrays + | '[' ']' arrays + { + uint32_t *new_array; + + $$ = $3; + + if (!(new_array = hlsl_realloc(ctx, $$.sizes, ($$.count + 1) * sizeof(*new_array)))) + { + vkd3d_free($$.sizes); + YYABORT; + } + + $$.sizes = new_array; + $$.sizes[$$.count++] = HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT; + } + var_modifiers: %empty { diff --git a/libs/vkd3d-shader/hlsl_sm1.c b/libs/vkd3d-shader/hlsl_sm1.c index 0cdd3917..30380fa2 100644 --- a/libs/vkd3d-shader/hlsl_sm1.c +++ b/libs/vkd3d-shader/hlsl_sm1.c @@ -234,6 +234,7 @@ static const struct hlsl_type *get_array_type(const struct hlsl_type *type) static unsigned int get_array_size(const struct hlsl_type *type) { + assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); if (type->type == HLSL_CLASS_ARRAY) return get_array_size(type->e.array.type) * type->e.array.elements_count; return 1; diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index 8afa9333..2dcfdb88 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c @@ -252,6 +252,7 @@ static const struct hlsl_type *get_array_type(const struct hlsl_type *type) static unsigned int get_array_size(const struct hlsl_type *type) { + assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); if (type->type == HLSL_CLASS_ARRAY) return get_array_size(type->e.array.type) * type->e.array.elements_count; return 1; diff --git a/tests/hlsl-initializer-implicit-array.shader_test b/tests/hlsl-initializer-implicit-array.shader_test index d2b94da4..91b6f049 100644 --- a/tests/hlsl-initializer-implicit-array.shader_test +++ b/tests/hlsl-initializer-implicit-array.shader_test @@ -6,8 +6,8 @@ float4 main() : SV_TARGET } [test] -todo draw quad -todo probe all rgba (50, 60, 70, 80) +draw quad +probe all rgba (50, 60, 70, 80) [pixel shader fail] -- 2.36.0
Hi, I think the patch generally looks good, but there are a few details to fix. Il 28/04/22 15:32, Giovanni Mascellani ha scritto:
@@ -328,6 +330,9 @@ struct hlsl_type *hlsl_new_array_type(struct hlsl_ctx *ctx, struct hlsl_type *ba
list_add_tail(&ctx->types, &type->entry);
+ if (type->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + TRACE("Implicit element count.\n"); +
I don't understand what this should be useful for, is it perhaps a leftover of development?
return type; }
@@ -410,6 +415,7 @@ unsigned int hlsl_type_component_count(struct hlsl_type *type) } if (type->type == HLSL_CLASS_ARRAY) { + assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); return hlsl_type_component_count(type->e.array.type) * type->e.array.elements_count; } if (type->type != HLSL_CLASS_STRUCT) @@ -1023,7 +1029,12 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru }
for (t = type; t->type == HLSL_CLASS_ARRAY; t = t->e.array.type) - vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count); + { + if (t->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + vkd3d_string_buffer_printf(string, "[<undefined>]"); + else + vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count); + }
I guess elements_count should never be IMPLICIT here, should it? As soon as the hunk in declare_vars() is done, all the counts are known. At any rate, if it can happen that the IMPLICIT branch can be taken here, then I'd just use "[]" instead of "[<undefined>]", which is the standard C way.
return string; }
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 28b2ff1b..94597999 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -227,6 +227,8 @@ struct hlsl_src
#define HLSL_MODIFIERS_MAJORITY_MASK (HLSL_MODIFIER_ROW_MAJOR | HLSL_MODIFIER_COLUMN_MAJOR)
+#define HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 0xffffffff + struct hlsl_reg_reservation { char type; diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 905dbfc5..e7fe74d8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
type = basic_type; for (i = 0; i < v->arrays.count; ++i) + { + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + unsigned int size = initializer_size(&v->initializer); + unsigned int elem_components = hlsl_type_component_count(type); + + assert(v->initializer.args_count);
Maybe it's just me, but here I would also assert that i == v->arrays.count - 1. This should already be forced by the parser, but it's a useful code documentation anyway.
+ v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;
Why "+ elem_components - 1", given that immediately below we error out if size is not a multiple of elem_components? Also, notice that elem_components could be zero. You should handle that gracefully.
+ + if (size % elem_components != 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Cannot initialize implicit array with %u components, expected a multiple of %u.", + size, elem_components); + free_parse_initializer(&v->initializer); + v->initializer.args_count = 0; + } + } type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); + } vkd3d_free(v->arrays.sizes);
Thanks, Giovanni.
Hello,
April 28, 2022 10:49 AM, "Giovanni Mascellani" <gmascellani(a)codeweavers.com> wrote:
> Hi,
>
> I think the patch generally looks good, but there are a few details to fix.
>
> Il 28/04/22 15:32, Giovanni Mascellani ha scritto:
>
>> @@ -328,6 +330,9 @@ struct hlsl_type *hlsl_new_array_type(struct hlsl_ctx *ctx, struct hlsl_type
>> *ba
>>> list_add_tail(&ctx->types, &type->entry);
>>> + if (type->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + TRACE("Implicit element count.\n");
>> +
>
> I don't understand what this should be useful for, is it perhaps a leftover of development?
>
Yes, probably a leftover. I think I was worried adding the special case of implicit arrays could
generate bugs in other parts of the code if they didn't consider it.
But that shouldn't be the case with this implementation since this special case is not present
after the initialization.
>> return type;
>> }
>>> @@ -410,6 +415,7 @@ unsigned int hlsl_type_component_count(struct hlsl_type *type)
>> }
>> if (type->type == HLSL_CLASS_ARRAY)
>> {
>> + assert(type->e.array.elements_count != HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT);
>> return hlsl_type_component_count(type->e.array.type) * type->e.array.elements_count;
>> }
>> if (type->type != HLSL_CLASS_STRUCT)
>> @@ -1023,7 +1029,12 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const
>> stru
>> }
>>> for (t = type; t->type == HLSL_CLASS_ARRAY; t = t->e.array.type)
>> - vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count);
>> + {
>> + if (t->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + vkd3d_string_buffer_printf(string, "[<undefined>]");
>> + else
>> + vkd3d_string_buffer_printf(string, "[%u]", t->e.array.elements_count);
>> + }
>
> I guess elements_count should never be IMPLICIT here, should it? As soon as the hunk in
> declare_vars() is done, all the counts are known.
>
Yes, but I think we better cover all cases in a function we may be using for debugging purposes.
> At any rate, if it can happen that the IMPLICIT branch can be taken here, then I'd just use "[]"
> instead of "[<undefined>]", which is the standard C way.
>
I think I opted for this verbose alternative since finding an implicit size array in the wild would
be an oddity worth emphasizing. But maybe I was paranoid. I will change it to "[]".
>> return string;
>> }
>>> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
>> index 28b2ff1b..94597999 100644
>> --- a/libs/vkd3d-shader/hlsl.h
>> +++ b/libs/vkd3d-shader/hlsl.h
>> @@ -227,6 +227,8 @@ struct hlsl_src
>>> #define HLSL_MODIFIERS_MAJORITY_MASK (HLSL_MODIFIER_ROW_MAJOR | HLSL_MODIFIER_COLUMN_MAJOR)
>>> +#define HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 0xffffffff
>> +
>> struct hlsl_reg_reservation
>> {
>> char type;
>> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
>> index 905dbfc5..e7fe74d8 100644
>> --- a/libs/vkd3d-shader/hlsl.y
>> +++ b/libs/vkd3d-shader/hlsl.y
>> @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type
>> *basic_t
>>> type = basic_type;
>> for (i = 0; i < v->arrays.count; ++i)
>> + {
>> + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + {
>> + unsigned int size = initializer_size(&v->initializer);
>> + unsigned int elem_components = hlsl_type_component_count(type);
>> +
>> + assert(v->initializer.args_count);
>
> Maybe it's just me, but here I would also assert that i == v->arrays.count - 1. This should already
> be forced by the parser, but it's a useful code documentation anyway.
>
Sounds good.
>> + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;
>
> Why "+ elem_components - 1", given that immediately below we error out if size is not a multiple of
> elem_components?
>
> Also, notice that elem_components could be zero. You should handle that gracefully.
>
These are good points, I will update the patch.
>> +
>> + if (size % elem_components != 0)
>> + {
>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
>> + "Cannot initialize implicit array with %u components, expected a multiple of %u.",
>> + size, elem_components);
>> + free_parse_initializer(&v->initializer);
>> + v->initializer.args_count = 0;
>> + }
>> + }
>> type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
>> + }
>> vkd3d_free(v->arrays.sizes);
>
> Thanks, Giovanni.
On 4/28/22 08:32, Giovanni Mascellani wrote:
@@ -328,6 +330,9 @@ struct hlsl_type *hlsl_new_array_type(struct hlsl_ctx *ctx, struct hlsl_type *ba
list_add_tail(&ctx->types, &type->entry);
+ if (type->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + TRACE("Implicit element count.\n"); + return type; }
When can this currently happen?
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 905dbfc5..e7fe74d8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
type = basic_type; for (i = 0; i < v->arrays.count; ++i) + { + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + unsigned int size = initializer_size(&v->initializer); + unsigned int elem_components = hlsl_type_component_count(type); + + assert(v->initializer.args_count); + + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components; + + if (size % elem_components != 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Cannot initialize implicit array with %u components, expected a multiple of %u.", + size, elem_components); + free_parse_initializer(&v->initializer); + v->initializer.args_count = 0; + }
This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially considering that the right thing is probably to fail compilation. It also won't do the right thing for initializers without braces (viz. also fail compilation). I think we need tests for all of these corner cases, and also a test for missing initializers.
+ } type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); + } vkd3d_free(v->arrays.sizes);
if (type->type != HLSL_CLASS_MATRIX) @@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %token <name> TYPE_IDENTIFIER
%type <arrays> arrays +%type <arrays> implicit_arrays
%type <assign_op> assign_op
@@ -3108,7 +3129,7 @@ variables_def: }
variable_decl: - any_identifier arrays colon_attribute + any_identifier implicit_arrays colon_attribute { $$ = hlsl_alloc(ctx, sizeof(*$$)); $$->loc = @1; @@ -3137,6 +3158,15 @@ state_block:
variable_def: variable_decl + { + if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Implicit array requires initializer."); + free_parse_variable_def($$); + YYABORT; + } + }
This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets (and no initializer). It also aborts compilation somewhat unnecessarily. (As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g. "Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.) As a special extra, this code is apparently valid, and I think deserves to be a test case: struct apple { Texture2D t[]; }; uniform struct apple a; float4 main() : sv_target { return a.t[0].Load(0); } [We don't currently have support for unbounded arrays in the shader_runner infrastructure, though, so it doesn't urgently need to be a test case. Alternatively it could be written in C.]
| variable_decl '=' complex_initializer { $$ = $1; @@ -3188,6 +3218,24 @@ arrays: $$.sizes[$$.count++] = size; }
+implicit_arrays: + arrays + | '[' ']' arrays + { + uint32_t *new_array; + + $$ = $3; + + if (!(new_array = hlsl_realloc(ctx, $$.sizes, ($$.count + 1) * sizeof(*new_array)))) + { + vkd3d_free($$.sizes); + YYABORT; + } + + $$.sizes = new_array; + $$.sizes[$$.count++] = HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT; + } +
I think this should be part of the "arrays" rule, actually. Besides being simpler YACC, it allows us to generate more helpful messages than "syntax error" if someone tries to use an empty array size in an invalid context.
Hello,
April 28, 2022 2:22 PM, "Zebediah Figura" <zfigura(a)codeweavers.com> wrote:
> On 4/28/22 08:32, Giovanni Mascellani wrote:
>
>> @@ -328,6 +330,9 @@ struct hlsl_type *hlsl_new_array_type(struct hlsl_ctx *ctx, struct hlsl_type
>> *ba
>>> list_add_tail(&ctx->types, &type->entry);
>>> + if (type->e.array.elements_count == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + TRACE("Implicit element count.\n");
>> +
>> return type;
>> }
>
> When can this currently happen?
>
Huh, actually this shouldn't happen in the current patch as it is at least. I will remove it.
>> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
>> index 905dbfc5..e7fe74d8 100644
>> --- a/libs/vkd3d-shader/hlsl.y
>> +++ b/libs/vkd3d-shader/hlsl.y
>> @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type
>> *basic_t
>>> type = basic_type;
>> for (i = 0; i < v->arrays.count; ++i)
>> + {
>> + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + {
>> + unsigned int size = initializer_size(&v->initializer);
>> + unsigned int elem_components = hlsl_type_component_count(type);
>> +
>> + assert(v->initializer.args_count);
>> +
>> + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;
>> +
>> + if (size % elem_components != 0)
>> + {
>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
>> + "Cannot initialize implicit array with %u components, expected a multiple of %u.",
>> + size, elem_components);
>> + free_parse_initializer(&v->initializer);
>> + v->initializer.args_count = 0;
>> + }
>
> This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially
> considering that the right thing is probably to fail compilation.
>
> It also won't do the right thing for initializers without braces (viz. also fail compilation).
>
These cases are checked in the parse rules introduced in this patch.
> I think we need tests for all of these corner cases, and also a test for missing initializers.
>
Okay, adding them.
>> + }
>> type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
>> + }
>> vkd3d_free(v->arrays.sizes);
>>> if (type->type != HLSL_CLASS_MATRIX)
>> @@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct
>> hl
>> %token <name> TYPE_IDENTIFIER
>>> %type <arrays> arrays
>> +%type <arrays> implicit_arrays
>>> %type <assign_op> assign_op
>>> @@ -3108,7 +3129,7 @@ variables_def:
>> }
>>> variable_decl:
>> - any_identifier arrays colon_attribute
>> + any_identifier implicit_arrays colon_attribute
>> {
>> $$ = hlsl_alloc(ctx, sizeof(*$$));
>> $$->loc = @1;
>> @@ -3137,6 +3158,15 @@ state_block:
>>> variable_def:
>> variable_decl
>> + {
>> + if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] ==
>> HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>> + {
>> + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER,
>> + "Implicit array requires initializer.");
>> + free_parse_variable_def($$);
>> + YYABORT;
>> + }
>> + }
>
> This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets
> (and no initializer).
>
I see, I didn't knew about those.
> It also aborts compilation somewhat unnecessarily.
>
AFAIK (except for unbounded resource arrays) the native compiler doesn't allow implicit size arrays
without initializer, so aborting seemed logical.
> (As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader
> model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g.
> "Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.)
>
> As a special extra, this code is apparently valid, and I think deserves to be a test case:
>
> struct apple
> {
> Texture2D t[];
> };
>
Hmm, this is interesting. Maybe it is intended for input semantics?
I will investigate.
> uniform struct apple a;
>
> float4 main() : sv_target
> {
> return a.t[0].Load(0);
> }
>
> [We don't currently have support for unbounded arrays in the shader_runner infrastructure, though,
> so it doesn't urgently need to be a test case. Alternatively it could be written in C.]
>
Okay, I will see if I can write a C test, if not now, later.
>> | variable_decl '=' complex_initializer
>> {
>> $$ = $1;
>> @@ -3188,6 +3218,24 @@ arrays:
>> $$.sizes[$$.count++] = size;
>> }
>>> +implicit_arrays:
>> + arrays
>> + | '[' ']' arrays
>> + {
>> + uint32_t *new_array;
>> +
>> + $$ = $3;
>> +
>> + if (!(new_array = hlsl_realloc(ctx, $$.sizes, ($$.count + 1) * sizeof(*new_array))))
>> + {
>> + vkd3d_free($$.sizes);
>> + YYABORT;
>> + }
>> +
>> + $$.sizes = new_array;
>> + $$.sizes[$$.count++] = HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT;
>> + }
>> +
>
> I think this should be part of the "arrays" rule, actually. Besides being simpler YACC, it allows
> us to generate more helpful messages than "syntax error" if someone tries to use an empty array
> size in an invalid context.
Okay, I will try moving everything outside the parser rules. But we will need to introduce several
checks to ensure that all the array sizes are defined where they are needed, including typedefs,
casts, struct members.
Now that I think about it, it may also be worth testing if implicit size arrays are allowed in function
argument and return types.
On 4/28/22 14:45, Francisco Casas wrote:
>>> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
>>> index 905dbfc5..e7fe74d8 100644
>>> --- a/libs/vkd3d-shader/hlsl.y
>>> +++ b/libs/vkd3d-shader/hlsl.y
>>> @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type
>>> *basic_t
>>>> type = basic_type;
>>> for (i = 0; i < v->arrays.count; ++i)
>>> + {
>>> + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>>> + {
>>> + unsigned int size = initializer_size(&v->initializer);
>>> + unsigned int elem_components = hlsl_type_component_count(type);
>>> +
>>> + assert(v->initializer.args_count);
>>> +
>>> + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;
>>> +
>>> + if (size % elem_components != 0)
>>> + {
>>> + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
>>> + "Cannot initialize implicit array with %u components, expected a multiple of %u.",
>>> + size, elem_components);
>>> + free_parse_initializer(&v->initializer);
>>> + v->initializer.args_count = 0;
>>> + }
>>
>> This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially
>> considering that the right thing is probably to fail compilation.
>>
>> It also won't do the right thing for initializers without braces (viz. also fail compilation).
>>
>
> These cases are checked in the parse rules introduced in this patch.
Oh, I see, I wasn't actually correctly reading the parser rule. That
makes sense.
On the other hand, handling IMPLICIT inside of the loop, without any
checks, was one of the things that made me think "we're not handling
inner arrays correctly", so arguably something deserves changing here :-)
I don't see any handling for braceless initializers in this patch,
though; am I missing something?
>
>> I think we need tests for all of these corner cases, and also a test for missing initializers.
>>
>
> Okay, adding them.
>
>>> + }
>>> type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
>>> + }
>>> vkd3d_free(v->arrays.sizes);
>>>> if (type->type != HLSL_CLASS_MATRIX)
>>> @@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct
>>> hl
>>> %token <name> TYPE_IDENTIFIER
>>>> %type <arrays> arrays
>>> +%type <arrays> implicit_arrays
>>>> %type <assign_op> assign_op
>>>> @@ -3108,7 +3129,7 @@ variables_def:
>>> }
>>>> variable_decl:
>>> - any_identifier arrays colon_attribute
>>> + any_identifier implicit_arrays colon_attribute
>>> {
>>> $$ = hlsl_alloc(ctx, sizeof(*$$));
>>> $$->loc = @1;
>>> @@ -3137,6 +3158,15 @@ state_block:
>>>> variable_def:
>>> variable_decl
>>> + {
>>> + if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] ==
>>> HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
>>> + {
>>> + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER,
>>> + "Implicit array requires initializer.");
>>> + free_parse_variable_def($$);
>>> + YYABORT;
>>> + }
>>> + }
>>
>> This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets
>> (and no initializer).
>>
>
> I see, I didn't knew about those.
>
>> It also aborts compilation somewhat unnecessarily.
>>
>
> AFAIK (except for unbounded resource arrays) the native compiler doesn't allow implicit size arrays
> without initializer, so aborting seemed logical.
Failing compilation is fine, but in general I think we want to avoid
aborting if we can help it. We should only abort if we really can't
continue parsing, e.g. if we encounter a syntax error. That way, if
there are multiple errors, the (HLSL) programmer can deal with them all
at once.
>
>> (As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader
>> model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g.
>> "Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.)
>>
>> As a special extra, this code is apparently valid, and I think deserves to be a test case:
>>
>> struct apple
>> {
>> Texture2D t[];
>> };
>>
>
> Hmm, this is interesting. Maybe it is intended for input semantics?
> I will investigate.
Not for input semantics, but rather for bound resources. Shader model
5.0 allows declaring arrays of textures, and shader model 5.1 (and
Direct3D 12) allows declaring "unbounded" arrays, as e.g. "Texture2D
t[]" or "Texture2D t[0]".
Normally textures aren't declared as part of a struct, but I was curious
to see if it was legal, and it turns out it is. (Although the reflection
type information that's generated isn't quite correct.)
April 28, 2022 4:22 PM, "Zebediah Figura" <zfigura(a)codeweavers.com (mailto:zfigura(a)codeweavers.com)> wrote: On 4/28/22 14:45, Francisco Casas wrote: diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 905dbfc5..e7fe74d8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t type = basic_type; for (i = 0; i < v->arrays.count; ++i) + { + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + unsigned int size = initializer_size(&v->initializer); + unsigned int elem_components = hlsl_type_component_count(type); + + assert(v->initializer.args_count); + + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components; + + if (size % elem_components != 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Cannot initialize implicit array with %u components, expected a multiple of %u.", + size, elem_components); + free_parse_initializer(&v->initializer); + v->initializer.args_count = 0; + } This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially considering that the right thing is probably to fail compilation. It also won't do the right thing for initializers without braces (viz. also fail compilation).These cases are checked in the parse rules introduced in this patch. Oh, I see, I wasn't actually correctly reading the parser rule. That makes sense. On the other hand, handling IMPLICIT inside of the loop, without any checks, was one of the things that made me think "we're not handling inner arrays correctly", so arguably something deserves changing here :-) Well, the structure of the parsing rule ensures that only the most external array could have IMPLICIT side. This is related to the assert(i == v->arrays.count - 1); suggested by Giovanni. I don't see any handling for braceless initializers in this patch, though; am I missing something? The error is added to the "variable_def:" rule. But then again, maybe it is not so clear that the parse rule ensures that only the most external array can have IMPLICIT size. I could add the same assertion here too. By the way, I am not sure if you changed your stance on checking the implicit size arrays at the parse level. I would prefer to keep it there... if I find a way of also handling these unbounded resource arrays nicely. I think we need tests for all of these corner cases, and also a test for missing initializers. Okay, adding them. + } type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); + } vkd3d_free(v->arrays.sizes); if (type->type != HLSL_CLASS_MATRIX) @@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %token <name> TYPE_IDENTIFIER %type <arrays> arrays +%type <arrays> implicit_arrays %type <assign_op> assign_op @@ -3108,7 +3129,7 @@ variables_def: } variable_decl: - any_identifier arrays colon_attribute + any_identifier implicit_arrays colon_attribute { $$ = hlsl_alloc(ctx, sizeof(*$$)); $$->loc = @1; @@ -3137,6 +3158,15 @@ state_block: variable_def: variable_decl + { + if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Implicit array requires initializer."); + free_parse_variable_def($$); + YYABORT; + } + } This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets (and no initializer).I see, I didn't knew about those. It also aborts compilation somewhat unnecessarily. AFAIK (except for unbounded resource arrays) the native compiler doesn't allow implicit size arrays without initializer, so aborting seemed logical. Failing compilation is fine, but in general I think we want to avoid aborting if we can help it. We should only abort if we really can't continue parsing, e.g. if we encounter a syntax error. That way, if there are multiple errors, the (HLSL) programmer can deal with them all at once. (As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g. "Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.) As a special extra, this code is apparently valid, and I think deserves to be a test case: struct apple { Texture2D t[]; };Hmm, this is interesting. Maybe it is intended for input semantics? I will investigate. Not for input semantics, but rather for bound resources. Shader model 5.0 allows declaring arrays of textures, and shader model 5.1 (and Direct3D 12) allows declaring "unbounded" arrays, as e.g. "Texture2D t[]" or "Texture2D t[0]". Normally textures aren't declared as part of a struct, but I was curious to see if it was legal, and it turns out it is. (Although the reflection type information that's generated isn't quite correct.) Hmm, I will see where I can add a FIXME for those cases for now then.
April 28, 2022 4:22 PM, "Zebediah Figura" <zfigura(a)codeweavers.com (mailto:zfigura(a)codeweavers.com)> wrote: On 4/28/22 14:45, Francisco Casas wrote: diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 905dbfc5..e7fe74d8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t type = basic_type; for (i = 0; i < v->arrays.count; ++i) + { + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + unsigned int size = initializer_size(&v->initializer); + unsigned int elem_components = hlsl_type_component_count(type); + + assert(v->initializer.args_count); + + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components; + + if (size % elem_components != 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Cannot initialize implicit array with %u components, expected a multiple of %u.", + size, elem_components); + free_parse_initializer(&v->initializer); + v->initializer.args_count = 0; + } This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially considering that the right thing is probably to fail compilation. It also won't do the right thing for initializers without braces (viz. also fail compilation).These cases are checked in the parse rules introduced in this patch. Oh, I see, I wasn't actually correctly reading the parser rule. That makes sense. On the other hand, handling IMPLICIT inside of the loop, without any checks, was one of the things that made me think "we're not handling inner arrays correctly", so arguably something deserves changing here :-) Well, the structure of the parsing rule ensures that only the most external array could have IMPLICIT side. This is related to the assert(i == v->arrays.count - 1); suggested by Giovanni. I don't see any handling for braceless initializers in this patch, though; am I missing something? The error is added to the "variable_def:" rule. But then again, maybe it is not so clear that the parse rule ensures that only the most external array can have IMPLICIT size. I could add the same assertion here too. By the way, I am not sure if you changed your stance on checking the implicit size arrays at the parse level. I would prefer to keep it there... if I find a way of also handling these unbounded resource arrays nicely. I think we need tests for all of these corner cases, and also a test for missing initializers. Okay, adding them. + } type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); + } vkd3d_free(v->arrays.sizes); if (type->type != HLSL_CLASS_MATRIX) @@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %token <name> TYPE_IDENTIFIER %type <arrays> arrays +%type <arrays> implicit_arrays %type <assign_op> assign_op @@ -3108,7 +3129,7 @@ variables_def: } variable_decl: - any_identifier arrays colon_attribute + any_identifier implicit_arrays colon_attribute { $$ = hlsl_alloc(ctx, sizeof(*$$)); $$->loc = @1; @@ -3137,6 +3158,15 @@ state_block: variable_def: variable_decl + { + if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Implicit array requires initializer."); + free_parse_variable_def($$); + YYABORT; + } + } This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets (and no initializer).I see, I didn't knew about those. It also aborts compilation somewhat unnecessarily. AFAIK (except for unbounded resource arrays) the native compiler doesn't allow implicit size arrays without initializer, so aborting seemed logical. Failing compilation is fine, but in general I think we want to avoid aborting if we can help it. We should only abort if we really can't continue parsing, e.g. if we encounter a syntax error. That way, if there are multiple errors, the (HLSL) programmer can deal with them all at once. (As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g. "Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.) As a special extra, this code is apparently valid, and I think deserves to be a test case: struct apple { Texture2D t[]; };Hmm, this is interesting. Maybe it is intended for input semantics? I will investigate. Not for input semantics, but rather for bound resources. Shader model 5.0 allows declaring arrays of textures, and shader model 5.1 (and Direct3D 12) allows declaring "unbounded" arrays, as e.g. "Texture2D t[]" or "Texture2D t[0]". Normally textures aren't declared as part of a struct, but I was curious to see if it was legal, and it turns out it is. (Although the reflection type information that's generated isn't quite correct.) Hmm, I will see where I can add a FIXME for those cases for now then.
April 28, 2022 4:22 PM, "Zebediah Figura" <zfigura(a)codeweavers.com> wrote:
On 4/28/22 14:45, Francisco Casas wrote:
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 905dbfc5..e7fe74d8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t type = basic_type; for (i = 0; i < v->arrays.count; ++i) + { + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + unsigned int size = initializer_size(&v->initializer); + unsigned int elem_components = hlsl_type_component_count(type); + + assert(v->initializer.args_count); + + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components; + + if (size % elem_components != 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Cannot initialize implicit array with %u components, expected a multiple of %u.", + size, elem_components); + free_parse_initializer(&v->initializer); + v->initializer.args_count = 0; + }
This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially considering that the right thing is probably to fail compilation.
It also won't do the right thing for initializers without braces (viz. also fail compilation).
These cases are checked in the parse rules introduced in this patch.
Oh, I see, I wasn't actually correctly reading the parser rule. That makes sense.
On the other hand, handling IMPLICIT inside of the loop, without any checks, was one of the things that made me think "we're not handling inner arrays correctly", so arguably something deserves changing here :-)
Well, the structure of the parsing rule ensures that only the most external array could have IMPLICIT side. This is related to the assert(i == v->arrays.count - 1); suggested by Giovanni.
I don't see any handling for braceless initializers in this patch, though; am I missing something?
The error is added to the "variable_def:" rule. But then again, maybe it is not so clear that the parse rule ensures that only the most external array can have IMPLICIT size. I could add the same assertion here too. By the way, I am not sure if you changed your stance on checking the implicit size arrays at the parse level. I would prefer to keep it there... if I find a way of also handling these unbounded resource arrays nicely.
I think we need tests for all of these corner cases, and also a test for missing initializers.
Okay, adding them. + } type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); + } vkd3d_free(v->arrays.sizes); if (type->type != HLSL_CLASS_MATRIX) @@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %token <name> TYPE_IDENTIFIER %type <arrays> arrays +%type <arrays> implicit_arrays %type <assign_op> assign_op @@ -3108,7 +3129,7 @@ variables_def: } variable_decl: - any_identifier arrays colon_attribute + any_identifier implicit_arrays colon_attribute { $$ = hlsl_alloc(ctx, sizeof(*$$)); $$->loc = @1; @@ -3137,6 +3158,15 @@ state_block: variable_def: variable_decl + { + if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Implicit array requires initializer.");
Here ^
+ free_parse_variable_def($$); + YYABORT; + } + }
This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets (and no initializer).
I see, I didn't knew about those. It also aborts compilation somewhat unnecessarily.
AFAIK (except for unbounded resource arrays) the native compiler doesn't allow implicit size arrays without initializer, so aborting seemed logical.
Failing compilation is fine, but in general I think we want to avoid aborting if we can help it. We should only abort if we really can't continue parsing, e.g. if we encounter a syntax error. That way, if there are multiple errors, the (HLSL) programmer can deal with them all at once.
(As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader
model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g. "Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.)
As a special extra, this code is apparently valid, and I think deserves to be a test case:
struct apple { Texture2D t[]; };
Hmm, this is interesting. Maybe it is intended for input semantics? I will investigate.
Not for input semantics, but rather for bound resources. Shader model 5.0 allows declaring arrays of textures, and shader model 5.1 (and Direct3D 12) allows declaring "unbounded" arrays, as e.g. "Texture2D t[]" or "Texture2D t[0]".
Normally textures aren't declared as part of a struct, but I was curious to see if it was legal, and it turns out it is. (Although the reflection type information that's generated isn't quite correct.)
Hmm, I will see where I can add a FIXME for those cases for now then.
On 4/28/22 16:30, Francisco Casas wrote:
April 28, 2022 4:22 PM, "Zebediah Figura" <zfigura(a)codeweavers.com> wrote:
On 4/28/22 14:45, Francisco Casas wrote:
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 905dbfc5..e7fe74d8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t type = basic_type; for (i = 0; i < v->arrays.count; ++i) + { + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + unsigned int size = initializer_size(&v->initializer); + unsigned int elem_components = hlsl_type_component_count(type); + + assert(v->initializer.args_count); + + v->arrays.sizes[i] = (size + elem_components - 1)/elem_components; + + if (size % elem_components != 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Cannot initialize implicit array with %u components, expected a multiple of %u.", + size, elem_components); + free_parse_initializer(&v->initializer); + v->initializer.args_count = 0; + }
This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially considering that the right thing is probably to fail compilation.
It also won't do the right thing for initializers without braces (viz. also fail compilation).
These cases are checked in the parse rules introduced in this patch.
Oh, I see, I wasn't actually correctly reading the parser rule. That makes sense.
On the other hand, handling IMPLICIT inside of the loop, without any checks, was one of the things that made me think "we're not handling inner arrays correctly", so arguably something deserves changing here :-)
Well, the structure of the parsing rule ensures that only the most external array could have IMPLICIT side. This is related to the assert(i == v->arrays.count - 1); suggested by Giovanni.
I don't see any handling for braceless initializers in this patch, though; am I missing something?
The error is added to the "variable_def:" rule. But then again, maybe it is not so clear that the parse rule ensures that only the most external array can have IMPLICIT size. I could add the same assertion here too.
Right, the assert works to clarify. I'm not sure whether "float a[2][]" should be a syntax error, though. As elsewhere, it'd be more helpful to explain why it's wrong, and probably also to avoid failing compilation immediately.
By the way, I am not sure if you changed your stance on checking the implicit size arrays at the parse level. I would prefer to keep it there... if I find a way of also handling these unbounded resource arrays nicely.
Well, it at least can't be done until we have type information, which means it'd need to be deferred until declare_vars and gen_struct_fields(). That sounds like conceptually the right place for it, to me.
I think we need tests for all of these corner cases, and also a test for missing initializers.
Okay, adding them. + } type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); + } vkd3d_free(v->arrays.sizes); if (type->type != HLSL_CLASS_MATRIX) @@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %token <name> TYPE_IDENTIFIER %type <arrays> arrays +%type <arrays> implicit_arrays %type <assign_op> assign_op @@ -3108,7 +3129,7 @@ variables_def: } variable_decl: - any_identifier arrays colon_attribute + any_identifier implicit_arrays colon_attribute { $$ = hlsl_alloc(ctx, sizeof(*$$)); $$->loc = @1; @@ -3137,6 +3158,15 @@ state_block: variable_def: variable_decl + { + if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) + { + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Implicit array requires initializer.");
Here ^
+ free_parse_variable_def($$); + YYABORT; + } + }
This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets (and no initializer).
I see, I didn't knew about those. It also aborts compilation somewhat unnecessarily.
AFAIK (except for unbounded resource arrays) the native compiler doesn't allow implicit size arrays without initializer, so aborting seemed logical.
Failing compilation is fine, but in general I think we want to avoid aborting if we can help it. We should only abort if we really can't continue parsing, e.g. if we encounter a syntax error. That way, if there are multiple errors, the (HLSL) programmer can deal with them all at once.
(As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader
model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g. "Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.)
As a special extra, this code is apparently valid, and I think deserves to be a test case:
struct apple { Texture2D t[]; };
Hmm, this is interesting. Maybe it is intended for input semantics? I will investigate.
Not for input semantics, but rather for bound resources. Shader model 5.0 allows declaring arrays of textures, and shader model 5.1 (and Direct3D 12) allows declaring "unbounded" arrays, as e.g. "Texture2D t[]" or "Texture2D t[0]".
Normally textures aren't declared as part of a struct, but I was curious to see if it was legal, and it turns out it is. (Although the reflection type information that's generated isn't quite correct.)
Hmm, I will see where I can add a FIXME for those cases for now then.
It's not something that this patch necessarily needs to handle, even with a FIXME, but it should inform how the code is structured, which is why I bring it up.
Based on a patch by Francisco Casas. Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index e7fe74d8..3fff8fc6 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2044,12 +2044,20 @@ static struct list *add_call(struct hlsl_ctx *ctx, const char *name, static struct list *add_constructor(struct hlsl_ctx *ctx, struct hlsl_type *type, struct parse_initializer *params, struct vkd3d_shader_location loc) { + unsigned int size = initializer_size(params); static unsigned int counter; struct hlsl_ir_load *load; struct hlsl_ir_var *var; unsigned int i, idx = 0; char name[23]; + if (hlsl_type_component_count(type) != size) + { + hlsl_error(ctx, &loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Expected %u components in constructor, but got %u.", hlsl_type_component_count(type), size); + return NULL; + } + sprintf(name, "<constructor-%x>", counter++); if (!(var = hlsl_new_synthetic_var(ctx, name, type, loc))) return NULL; -- 2.36.0
On 4/28/22 08:32, Giovanni Mascellani wrote:
Based on a patch by Francisco Casas.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index e7fe74d8..3fff8fc6 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2044,12 +2044,20 @@ static struct list *add_call(struct hlsl_ctx *ctx, const char *name, static struct list *add_constructor(struct hlsl_ctx *ctx, struct hlsl_type *type, struct parse_initializer *params, struct vkd3d_shader_location loc) { + unsigned int size = initializer_size(params); static unsigned int counter; struct hlsl_ir_load *load; struct hlsl_ir_var *var; unsigned int i, idx = 0; char name[23];
+ if (hlsl_type_component_count(type) != size) + { + hlsl_error(ctx, &loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Expected %u components in constructor, but got %u.", hlsl_type_component_count(type), size); + return NULL; + } + sprintf(name, "<constructor-%x>", counter++); if (!(var = hlsl_new_synthetic_var(ctx, name, type, loc))) return NULL;
We already handle this in the caller.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 3fff8fc6..b68d9700 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2077,7 +2077,7 @@ static struct list *add_constructor(struct hlsl_ctx *ctx, struct hlsl_type *type continue; } - initialize_var_components(ctx, params->instrs, var, &idx, arg, &loc); + initialize_var_components(ctx, params->instrs, var, &idx, arg, &arg->loc); } if (!(load = hlsl_new_var_load(ctx, var, loc))) -- 2.36.0
On 4/28/22 08:32, Giovanni Mascellani wrote:
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 3fff8fc6..b68d9700 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2077,7 +2077,7 @@ static struct list *add_constructor(struct hlsl_ctx *ctx, struct hlsl_type *type continue; }
- initialize_var_components(ctx, params->instrs, var, &idx, arg, &loc); + initialize_var_components(ctx, params->instrs, var, &idx, arg, &arg->loc); }
if (!(load = hlsl_new_var_load(ctx, var, loc)))
Note that this makes the "loc" parameter to initialize_var_components() redundant. I think this patch makes sense, although do you happen to have an example of an error message that's improved this way?
Hi, Il 28/04/22 20:21, Zebediah Figura ha scritto:
Note that this makes the "loc" parameter to initialize_var_components() redundant.
Right. If that's ok for you, I'd fix it in a follow up.
I think this patch makes sense, although do you happen to have an example of an error message that's improved this way?
No, not really. Ideally initialize_var_components() should never fail, except for memory allocation, for which loc doesn't make much sense any way. My idea is to keep the code "sensible" even when that isn't expected to be apparent to the user. No big deal if the patch is dropped. Giovanni.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_sm4.c | 6 ------ tests/hlsl-initializer-matrix.shader_test | 2 +- tests/hlsl-matrix-indexing.shader_test | 2 +- tests/hlsl-numeric-types.shader_test | 4 ++-- 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index 2dcfdb88..6b98ca5d 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c @@ -2048,12 +2048,6 @@ static void write_sm4_store(struct hlsl_ctx *ctx, struct sm4_instruction instr; unsigned int writemask; - if (store->lhs.var->data_type->type == HLSL_CLASS_MATRIX) - { - hlsl_fixme(ctx, &store->node.loc, "Store to a matrix variable."); - return; - } - memset(&instr, 0, sizeof(instr)); instr.opcode = VKD3D_SM4_OP_MOV; diff --git a/tests/hlsl-initializer-matrix.shader_test b/tests/hlsl-initializer-matrix.shader_test index 790d402d..ea9de9c0 100644 --- a/tests/hlsl-initializer-matrix.shader_test +++ b/tests/hlsl-initializer-matrix.shader_test @@ -68,5 +68,5 @@ float4 main() : SV_TARGET } [test] -todo draw quad +draw quad probe all rgba (21, 22, 31, 32) diff --git a/tests/hlsl-matrix-indexing.shader_test b/tests/hlsl-matrix-indexing.shader_test index 9336a67a..716b43d4 100644 --- a/tests/hlsl-matrix-indexing.shader_test +++ b/tests/hlsl-matrix-indexing.shader_test @@ -74,5 +74,5 @@ float4 main() : SV_TARGET } [test] -todo draw quad +draw quad probe all rgba (1.0, 5.0, 7.0, 12.0) diff --git a/tests/hlsl-numeric-types.shader_test b/tests/hlsl-numeric-types.shader_test index 58612191..7504f95a 100644 --- a/tests/hlsl-numeric-types.shader_test +++ b/tests/hlsl-numeric-types.shader_test @@ -56,7 +56,7 @@ float4 main() : sv_target } [test] -todo draw quad +draw quad probe all rgba (5.0, 6.0, 7.0, 8.0) [pixel shader] @@ -68,7 +68,7 @@ float4 main() : sv_target } [test] -todo draw quad +draw quad probe all rgba (5.0, 6.0, 7.0, 0.0) [pixel shader fail] -- 2.36.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 179 +++++++++++++++++++++++++++---- 1 file changed, 159 insertions(+), 20 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index f7396a96..b967a2a3 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -273,8 +273,16 @@ static bool lower_broadcasts(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, v return false; } +enum copy_propagation_value_state +{ + VALUE_STATE_NOT_WRITTEN = 0, + VALUE_STATE_STATICALLY_WRITTEN, + VALUE_STATE_DYNAMICALLY_WRITTEN, +}; + struct copy_propagation_value { + enum copy_propagation_value_state state; struct hlsl_ir_node *node; unsigned int component; }; @@ -289,6 +297,7 @@ struct copy_propagation_var_def struct copy_propagation_state { struct rb_tree var_defs; + struct copy_propagation_state *parent; }; static int copy_propagation_var_def_compare(const void *key, const struct rb_entry *entry) @@ -306,15 +315,32 @@ static void copy_propagation_var_def_destroy(struct rb_entry *entry, void *conte vkd3d_free(var_def); } -static struct copy_propagation_var_def *copy_propagation_get_var_def(const struct copy_propagation_state *state, - const struct hlsl_ir_var *var) +static struct copy_propagation_value *copy_propagation_get_value(const struct copy_propagation_state *state, + const struct hlsl_ir_var *var, unsigned component) { - struct rb_entry *entry = rb_get(&state->var_defs, var); + for (; state; state = state->parent) + { + struct rb_entry *entry = rb_get(&state->var_defs, var); + if (entry) + { + struct copy_propagation_var_def *var_def = RB_ENTRY_VALUE(entry, struct copy_propagation_var_def, entry); + enum copy_propagation_value_state state = var_def->values[component].state; - if (entry) - return RB_ENTRY_VALUE(entry, struct copy_propagation_var_def, entry); - else - return NULL; + assert(component < var_def->var->data_type->reg_size); + + switch (state) + { + case VALUE_STATE_STATICALLY_WRITTEN: + return &var_def->values[component]; + case VALUE_STATE_DYNAMICALLY_WRITTEN: + return NULL; + case VALUE_STATE_NOT_WRITTEN: + break; + } + } + } + + return NULL; } static struct copy_propagation_var_def *copy_propagation_create_var_def(struct hlsl_ctx *ctx, @@ -338,10 +364,28 @@ static struct copy_propagation_var_def *copy_propagation_create_var_def(struct h return var_def; } +static void copy_propagation_invalidate_variable(struct copy_propagation_var_def *var_def, + unsigned offset, unsigned char writemask) +{ + unsigned i; + + TRACE("Invalidate variable %s[%u]%s.\n", var_def->var->name, offset, debug_hlsl_writemask(writemask)); + + for (i = 0; i < 4; ++i) + { + if (writemask & (1u << i)) + var_def->values[offset + i].state = VALUE_STATE_DYNAMICALLY_WRITTEN; + } +} + static void copy_propagation_invalidate_whole_variable(struct copy_propagation_var_def *var_def) { + unsigned i; + TRACE("Invalidate variable %s.\n", var_def->var->name); - memset(var_def->values, 0, sizeof(*var_def->values) * var_def->var->data_type->reg_size); + + for (i = 0; i < var_def->var->data_type->reg_size; ++i) + var_def->values[i].state = VALUE_STATE_DYNAMICALLY_WRITTEN; } static void copy_propagation_set_value(struct copy_propagation_var_def *var_def, unsigned int offset, @@ -355,6 +399,7 @@ static void copy_propagation_set_value(struct copy_propagation_var_def *var_def, { TRACE("Variable %s[%u] is written by instruction %p%s.\n", var_def->var->name, offset + i, node, debug_hlsl_writemask(1u << i)); + var_def->values[offset + i].state = VALUE_STATE_STATICALLY_WRITTEN; var_def->values[offset + i].node = node; var_def->values[offset + i].component = j++; } @@ -366,32 +411,34 @@ static struct hlsl_ir_node *copy_propagation_compute_replacement(struct hlsl_ctx unsigned int count, unsigned int *swizzle) { const struct hlsl_ir_var *var = deref->var; - struct copy_propagation_var_def *var_def; struct hlsl_ir_node *node = NULL; unsigned int offset, i; if (!hlsl_offset_from_deref(ctx, deref, &offset)) return NULL; - if (!(var_def = copy_propagation_get_var_def(state, var))) - return NULL; - - assert(offset + count <= var_def->var->data_type->reg_size); + if (var->data_type->type != HLSL_CLASS_OBJECT) + assert(offset + count <= var->data_type->reg_size); *swizzle = 0; for (i = 0; i < count; ++i) { + struct copy_propagation_value *value = copy_propagation_get_value(state, var, offset + i); + + if (!value) + return NULL; + if (!node) { - node = var_def->values[offset + i].node; + node = value->node; } - else if (node != var_def->values[offset + i].node) + else if (node != value->node) { TRACE("No single source for propagating load from %s[%u-%u].\n", var->name, offset, offset + count); return NULL; } - *swizzle |= var_def->values[offset + i].component << i * 2; + *swizzle |= value->component << i * 2; } TRACE("Load from %s[%u-%u] propagated as instruction %p%s.\n", @@ -495,6 +542,98 @@ static void copy_propagation_record_store(struct hlsl_ctx *ctx, struct hlsl_ir_s } } +static void copy_propagation_state_init(struct hlsl_ctx *ctx, struct copy_propagation_state *state, + struct copy_propagation_state *parent) +{ + rb_init(&state->var_defs, copy_propagation_var_def_compare); + state->parent = parent; +} + +static void copy_propagation_state_destroy(struct copy_propagation_state *state) +{ + rb_destroy(&state->var_defs, copy_propagation_var_def_destroy, NULL); +} + +static void copy_propagation_invalidate_from_block(struct hlsl_ctx *ctx, struct copy_propagation_state *state, + struct hlsl_block *block) +{ + struct hlsl_ir_node *instr; + + LIST_FOR_EACH_ENTRY(instr, &block->instrs, struct hlsl_ir_node, entry) + { + switch (instr->type) + { + case HLSL_IR_STORE: + { + struct hlsl_ir_store *store = hlsl_ir_store(instr); + struct copy_propagation_var_def *var_def; + struct hlsl_deref *lhs = &store->lhs; + struct hlsl_ir_var *var = lhs->var; + unsigned int offset; + + if (!(var_def = copy_propagation_create_var_def(ctx, state, var))) + continue; + + if (hlsl_offset_from_deref(ctx, lhs, &offset)) + copy_propagation_invalidate_variable(var_def, offset, store->writemask); + else + copy_propagation_invalidate_whole_variable(var_def); + + break; + } + + case HLSL_IR_IF: + { + struct hlsl_ir_if *iff = hlsl_ir_if(instr); + + copy_propagation_invalidate_from_block(ctx, state, &iff->then_instrs); + copy_propagation_invalidate_from_block(ctx, state, &iff->else_instrs); + + break; + } + + case HLSL_IR_LOOP: + { + struct hlsl_ir_loop *loop = hlsl_ir_loop(instr); + + copy_propagation_invalidate_from_block(ctx, state, &loop->body); + + break; + } + + default: + break; + } + } +} + +static bool copy_propagation_transform_block(struct hlsl_ctx *ctx, struct hlsl_block *block, + struct copy_propagation_state *state); + +static bool copy_propagation_process_if(struct hlsl_ctx *ctx, struct hlsl_ir_if *iff, + struct copy_propagation_state *state) +{ + struct copy_propagation_state inner_state; + bool progress = false; + + copy_propagation_state_init(ctx, &inner_state, state); + progress |= copy_propagation_transform_block(ctx, &iff->then_instrs, &inner_state); + copy_propagation_state_destroy(&inner_state); + + copy_propagation_state_init(ctx, &inner_state, state); + progress |= copy_propagation_transform_block(ctx, &iff->else_instrs, &inner_state); + copy_propagation_state_destroy(&inner_state); + + /* Ideally we'd invalidate the outer state looking at what was + * touched in the two inner states, but this doesn't work for + * loops (because we need to know what is invalidated in advance), + * so we need copy_propagation_invalidate_from_block() anyway. */ + copy_propagation_invalidate_from_block(ctx, state, &iff->then_instrs); + copy_propagation_invalidate_from_block(ctx, state, &iff->else_instrs); + + return progress; +} + static bool copy_propagation_transform_block(struct hlsl_ctx *ctx, struct hlsl_block *block, struct copy_propagation_state *state) { @@ -518,8 +657,8 @@ static bool copy_propagation_transform_block(struct hlsl_ctx *ctx, struct hlsl_b break; case HLSL_IR_IF: - FIXME("Copy propagation doesn't support conditionals yet, leaving.\n"); - return progress; + progress |= copy_propagation_process_if(ctx, hlsl_ir_if(instr), state); + break; case HLSL_IR_LOOP: FIXME("Copy propagation doesn't support loops yet, leaving.\n"); @@ -538,11 +677,11 @@ static bool copy_propagation_execute(struct hlsl_ctx *ctx, struct hlsl_block *bl struct copy_propagation_state state; bool progress; - rb_init(&state.var_defs, copy_propagation_var_def_compare); + copy_propagation_state_init(ctx, &state, NULL); progress = copy_propagation_transform_block(ctx, block, &state); - rb_destroy(&state.var_defs, copy_propagation_var_def_destroy, NULL); + copy_propagation_state_destroy(&state); return progress; } -- 2.36.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
On 4/28/22 08:31, Giovanni Mascellani wrote:
From: Francisco Casas <fcasas(a)codeweavers.com>
Otherwise we can get failed assertions: assert(node->type == HLSL_IR_LOAD); because broadcasts to these types are not implemented yet.
Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index e945b94d..f7396a96 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -637,6 +637,12 @@ static bool split_array_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, element_type = type->e.array.type; element_size = hlsl_type_get_array_element_reg_size(element_type);
+ if (rhs->type != HLSL_IR_LOAD) + { + hlsl_fixme(ctx, &instr->loc, "Array store rhs is not HLSL_IR_LOAD. Broadcast may be missing."); + return false; + } + for (i = 0; i < type->e.array.elements_count; ++i) { if (!split_copy(ctx, store, hlsl_ir_load(rhs), i * element_size, element_type))
In the future, though, I think we want to try to handle this in the place where we generate the invalid IR, if that's feasible. That makes it more inherently clear why we're generating invalid IR, and it also ensures we don't forget to remove the hlsl_fixme later. In this case I believe it'd be a matter of handling it for explicit and implicit casts.
participants (5)
-
Francisco Casas -
Giovanni Mascellani -
Henri Verbeet -
Zebediah Figura -
Zebediah Figura (she/her)