[PATCH v2 0/5] MR250: vkd3d-shader/hlsl: Declare vars individually when parsing declarations.
In a declaration with multiple variables, the variables must be created before the initializer of the next variable is parsed. This is required for initializers such as: ``` float a = 1, b = a, c = b + 1; ``` The solution is to parse the type information in the same rule as the first variable ('a' in this case) so that it can be declared before the parser reaches the initializer for the following variables. To initialize the following variables, the type information is passed along from the first variable's struct parse_variable_def to the next and so on. --- Solves https://bugs.winehq.org/show_bug.cgi?id=54484, pointed out by @nsivov. -- v2: vkd3d-shader/hlsl: Declare vars individually when parsing struct declarations. vkd3d-shader/hlsl: Declare vars individually when parsing regular declarations. vkd3d-shader/hlsl: Split declare_vars(). tests: Test multiple variable initializers. tests: Test semantics in uniform variables. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
From: Francisco Casas <fcasas(a)codeweavers.com> --- Makefile.am | 1 + tests/hlsl-uniform-semantics.shader_test | 28 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 tests/hlsl-uniform-semantics.shader_test diff --git a/Makefile.am b/Makefile.am index 92c6d0792..69833e9ce 100644 --- a/Makefile.am +++ b/Makefile.am @@ -131,6 +131,7 @@ vkd3d_shader_tests = \ tests/hlsl-transpose.shader_test \ tests/hlsl-trunc.shader_test \ tests/hlsl-type-names.shader_test \ + tests/hlsl-uniform-semantics.shader_test \ tests/hlsl-vector-indexing.shader_test \ tests/hlsl-vector-indexing-uniform.shader_test \ tests/lit.shader_test \ diff --git a/tests/hlsl-uniform-semantics.shader_test b/tests/hlsl-uniform-semantics.shader_test new file mode 100644 index 000000000..1125117fe --- /dev/null +++ b/tests/hlsl-uniform-semantics.shader_test @@ -0,0 +1,28 @@ +% Semantics can be used in uniforms even though they are ignored. + +[pixel shader] +float a : SEMANTIC; + +float4 main() : sv_target +{ + return a; +} + +[test] +uniform 0 float 3.5 +draw quad +probe all rgba (3.5, 3.5, 3.5, 3.5) + + +[pixel shader] +float2 a : sv_position; + +float4 main() : sv_target +{ + return float4(a, a); +} + +[test] +uniform 0 float4 4.0 5.0 6.0 7.0 +draw quad +probe all rgba (4.0, 5.0, 4.0, 5.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
From: Francisco Casas <fcasas(a)codeweavers.com> --- Makefile.am | 1 + tests/hlsl-initializer-multi.shader_test | 52 ++++++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 tests/hlsl-initializer-multi.shader_test diff --git a/Makefile.am b/Makefile.am index 69833e9ce..827f427c6 100644 --- a/Makefile.am +++ b/Makefile.am @@ -97,6 +97,7 @@ vkd3d_shader_tests = \ tests/hlsl-initializer-invalid-arg-count.shader_test \ tests/hlsl-initializer-local-array.shader_test \ tests/hlsl-initializer-matrix.shader_test \ + tests/hlsl-initializer-multi.shader_test \ tests/hlsl-initializer-nested.shader_test \ tests/hlsl-initializer-numeric.shader_test \ tests/hlsl-initializer-objects.shader_test \ diff --git a/tests/hlsl-initializer-multi.shader_test b/tests/hlsl-initializer-multi.shader_test new file mode 100644 index 000000000..9c45a5330 --- /dev/null +++ b/tests/hlsl-initializer-multi.shader_test @@ -0,0 +1,52 @@ +[pixel shader todo] +float4 main() : sv_target +{ + float a = 2.0, b = a + 1.0, c = b; + + return float4(a, b, c, 0); +} + +[test] +todo draw quad +probe all rgba (2, 3, 3, 0) + + +[pixel shader fail] +float4 main() : sv_target +{ + float a = 2.0, b = c + 1.0, c = b; + + return float4(0); +} + + +[pixel shader todo] +float4 main() : sv_target +{ + struct apple { + float a; + int b; + } apple1 = {7.2, 8.1}, apple2 = apple1; + + return float4(apple1.a, apple1.b, apple2.a, apple2.b); +} + +[test] +todo draw quad +probe all rgba (7.2, 8.0, 7.2, 8.0) + + +[pixel shader todo] +float4 main() : sv_target +{ + struct apple { + float a; + int b; + } apple1 = {5.2, 9.1}, apples[2] = {apple1, apple1}; + + return float4(apples[0].a, apples[0].b, apples[1].a, apples[1].b); +} + +[test] +todo draw quad +probe all rgba (5.2, 9.0, 5.2, 9.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
From: Francisco Casas <fcasas(a)codeweavers.com> Basically, declare_vars() is separated in two functions: 1. declare_var(), which now handles one variable at the time and doesn't free any memory. 2. initialize_vars(), which takes care of preparing the initialization instructions of several variables and frees their struct parse_variable_def, using exclusively free_parse_variable_def(). This allows to declare variables individually before the initializer of the next variable in the same declaration is parsed, which is used in the following patches. Also, simplifies memory management. --- libs/vkd3d-shader/hlsl.y | 363 ++++++++++++++++++++++----------------- 1 file changed, 202 insertions(+), 161 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 0e07fe578..96f3c8b52 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1924,31 +1924,24 @@ static bool type_has_numeric_components(struct hlsl_type *type) return false; } -static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_type, - unsigned int modifiers, const struct vkd3d_shader_location *modifiers_loc, struct list *var_list) +static void declare_var(struct hlsl_ctx *ctx, struct hlsl_type *basic_type, + unsigned int modifiers, const struct vkd3d_shader_location *modifiers_loc, struct parse_variable_def *v) { - struct parse_variable_def *v, *v_next; struct hlsl_ir_function_decl *func; + struct hlsl_semantic new_semantic; + bool unbounded_res_array = false; unsigned int invalid_modifiers; - struct list *statements_list; struct hlsl_ir_var *var; struct hlsl_type *type; bool local = true; + char *var_name; + unsigned int i; + + assert(basic_type); if (basic_type->class == HLSL_CLASS_MATRIX) assert(basic_type->modifiers & HLSL_MODIFIERS_MAJORITY_MASK); - if (!(statements_list = make_empty_list(ctx))) - { - LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) - free_parse_variable_def(v); - vkd3d_free(var_list); - return NULL; - } - - if (!var_list) - return statements_list; - invalid_modifiers = modifiers & (HLSL_STORAGE_IN | HLSL_STORAGE_OUT); if (invalid_modifiers) { @@ -1960,175 +1953,201 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t hlsl_release_string_buffer(ctx, string); } - LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) - { - bool unbounded_res_array = false; - unsigned int i; + type = basic_type; - type = basic_type; + if (shader_is_sm_5_1(ctx) && type->class == HLSL_CLASS_OBJECT) + { + for (i = 0; i < v->arrays.count; ++i) + unbounded_res_array |= (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); + } - if (shader_is_sm_5_1(ctx) && type->class == HLSL_CLASS_OBJECT) + if (unbounded_res_array) + { + if (v->arrays.count == 1) { - for (i = 0; i < v->arrays.count; ++i) - unbounded_res_array |= (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT); + hlsl_fixme(ctx, &v->loc, "Unbounded resource arrays."); + return; } - - if (unbounded_res_array) + else { - if (v->arrays.count == 1) - { - hlsl_fixme(ctx, &v->loc, "Unbounded resource arrays."); - free_parse_variable_def(v); - continue; - } - else - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Unbounded resource arrays cannot be multi-dimensional."); - } + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Unbounded resource arrays cannot be multi-dimensional."); } - else + } + else + { + for (i = 0; i < v->arrays.count; ++i) { - for (i = 0; i < v->arrays.count; ++i) + if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT) { - 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); - - if (i < v->arrays.count - 1) - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Only innermost array size can be implicit."); - free_parse_initializer(&v->initializer); - v->initializer.args_count = 0; - } - else if (elem_components == 0) - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Cannot declare an implicit size array of a size 0 type."); - free_parse_initializer(&v->initializer); - v->initializer.args_count = 0; - } - else if (size == 0) - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Implicit size arrays need to be initialized."); - free_parse_initializer(&v->initializer); - v->initializer.args_count = 0; + unsigned int size = initializer_size(&v->initializer); + unsigned int elem_components = hlsl_type_component_count(type); - } - else if (size % elem_components != 0) - { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, - "Cannot initialize implicit size array with %u components, expected a multiple of %u.", - size, elem_components); - free_parse_initializer(&v->initializer); - v->initializer.args_count = 0; - } - else - { - v->arrays.sizes[i] = size / elem_components; - } + if (i < v->arrays.count - 1) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Only innermost array size can be implicit."); + v->initializer.args_count = 0; + } + else if (elem_components == 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Cannot declare an implicit size array of a size 0 type."); + v->initializer.args_count = 0; + } + else if (size == 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, + "Implicit size arrays need to be initialized."); + v->initializer.args_count = 0; + } + else if (size % elem_components != 0) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Cannot initialize implicit size array with %u components, expected a multiple of %u.", + size, elem_components); + v->initializer.args_count = 0; + } + else + { + v->arrays.sizes[i] = size / elem_components; } - type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); } + type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]); } - vkd3d_free(v->arrays.sizes); - - if (!(var = hlsl_new_var(ctx, v->name, type, &v->loc, &v->semantic, modifiers, &v->reg_reservation))) - { - free_parse_variable_def(v); - continue; - } + } - var->buffer = ctx->cur_buffer; + if (!(var_name = vkd3d_strdup(v->name))) + return; - if (var->buffer == ctx->globals_buffer) + new_semantic = v->semantic; + if (v->semantic.name) + { + if (!(new_semantic.name = vkd3d_strdup(v->semantic.name))) { - if (var->reg_reservation.offset_type) - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, - "packoffset() is only allowed inside constant buffer declarations."); + vkd3d_free(var_name); + return; } + } - if (ctx->cur_scope == ctx->globals) - { - local = false; + if (!(var = hlsl_new_var(ctx, var_name, type, &v->loc, &new_semantic, modifiers, &v->reg_reservation))) + { + hlsl_cleanup_semantic(&new_semantic); + vkd3d_free(var_name); + return; + } - if ((modifiers & HLSL_STORAGE_UNIFORM) && (modifiers & HLSL_STORAGE_STATIC)) - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Variable '%s' is declared as both \"uniform\" and \"static\".", var->name); + var->buffer = ctx->cur_buffer; - /* Mark it as uniform. We need to do this here since synthetic - * variables also get put in the global scope, but shouldn't be - * considered uniforms, and we have no way of telling otherwise. */ - if (!(modifiers & HLSL_STORAGE_STATIC)) - var->storage_modifiers |= HLSL_STORAGE_UNIFORM; + if (var->buffer == ctx->globals_buffer) + { + if (var->reg_reservation.offset_type) + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "packoffset() is only allowed inside constant buffer declarations."); + } - if (ctx->profile->major_version < 5 && (var->storage_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 (ctx->cur_scope == ctx->globals) + { + local = false; - if ((func = hlsl_get_func_decl(ctx, var->name))) - { - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, - "'%s' is already defined as a function.", var->name); - hlsl_note(ctx, &func->loc, VKD3D_SHADER_LOG_ERROR, - "'%s' was previously defined here.", var->name); - } - } - else - { - static const unsigned int invalid = HLSL_STORAGE_EXTERN | HLSL_STORAGE_SHARED - | HLSL_STORAGE_GROUPSHARED | HLSL_STORAGE_UNIFORM; + if ((modifiers & HLSL_STORAGE_UNIFORM) && (modifiers & HLSL_STORAGE_STATIC)) + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, + "Variable '%s' is declared as both \"uniform\" and \"static\".", var->name); - if (modifiers & invalid) - { - struct vkd3d_string_buffer *string; + /* Mark it as uniform. We need to do this here since synthetic + * variables also get put in the global scope, but shouldn't be + * considered uniforms, and we have no way of telling otherwise. */ + if (!(modifiers & HLSL_STORAGE_STATIC)) + var->storage_modifiers |= HLSL_STORAGE_UNIFORM; - if ((string = hlsl_modifiers_to_string(ctx, modifiers & invalid))) - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Modifiers '%s' are not allowed on local variables.", string->buffer); - hlsl_release_string_buffer(ctx, string); - } - if (var->semantic.name) - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SEMANTIC, - "Semantics are not allowed on local variables."); + if (ctx->profile->major_version < 5 && (var->storage_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 ((var->storage_modifiers & HLSL_STORAGE_STATIC) && type_has_numeric_components(var->data_type) - && type_has_object_components(var->data_type, false)) + if ((func = hlsl_get_func_decl(ctx, var->name))) { - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Static variables cannot have both numeric and resource components."); + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, + "'%s' is already defined as a function.", var->name); + hlsl_note(ctx, &func->loc, VKD3D_SHADER_LOG_ERROR, + "'%s' was previously defined here.", var->name); } + } + else + { + static const unsigned int invalid = HLSL_STORAGE_EXTERN | HLSL_STORAGE_SHARED + | HLSL_STORAGE_GROUPSHARED | HLSL_STORAGE_UNIFORM; - if ((type->modifiers & HLSL_MODIFIER_CONST) && !v->initializer.args_count - && !(modifiers & (HLSL_STORAGE_STATIC | HLSL_STORAGE_UNIFORM))) + if (modifiers & invalid) { - hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, - "Const variable \"%s\" is missing an initializer.", var->name); - hlsl_free_var(var); - free_parse_initializer(&v->initializer); - vkd3d_free(v); - continue; + struct vkd3d_string_buffer *string; + + if ((string = hlsl_modifiers_to_string(ctx, modifiers & invalid))) + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, + "Modifiers '%s' are not allowed on local variables.", string->buffer); + hlsl_release_string_buffer(ctx, string); } + if (var->semantic.name) + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SEMANTIC, + "Semantics are not allowed on local variables."); + } + + if ((var->storage_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))) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Const variable \"%s\" is missing an initializer.", var->name); + hlsl_free_var(var); + return; + } + + if (!hlsl_add_var(ctx, var, local)) + { + struct hlsl_ir_var *old = hlsl_get_var(ctx->cur_scope, var->name); - if (!hlsl_add_var(ctx, var, local)) + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, + "Variable \"%s\" was already declared in this scope.", var->name); + hlsl_note(ctx, &old->loc, VKD3D_SHADER_LOG_ERROR, "\"%s\" was previously declared here.", old->name); + hlsl_free_var(var); + return; + } +} + +static struct list *initialize_vars(struct hlsl_ctx *ctx, struct list *var_list) +{ + struct parse_variable_def *v, *v_next; + struct list *statements_list; + struct hlsl_ir_var *var; + struct hlsl_type *type; + + if (!(statements_list = make_empty_list(ctx))) + { + LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) { - struct hlsl_ir_var *old = hlsl_get_var(ctx->cur_scope, var->name); + free_parse_variable_def(v); + } + vkd3d_free(var_list); + return NULL; + } - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, - "Variable \"%s\" was already declared in this scope.", var->name); - hlsl_note(ctx, &old->loc, VKD3D_SHADER_LOG_ERROR, "\"%s\" was previously declared here.", old->name); - hlsl_free_var(var); - free_parse_initializer(&v->initializer); - vkd3d_free(v); + LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) + { + /* If this fails, the variable failed to be declared. */ + if (!(var = hlsl_get_var(ctx->cur_scope, v->name))) + { + free_parse_variable_def(v); continue; } + type = var->data_type; if (v->initializer.args_count) { @@ -2143,8 +2162,7 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, "Expected %u components in initializer, but got %u.", hlsl_type_component_count(type), size); - free_parse_initializer(&v->initializer); - vkd3d_free(v); + free_parse_variable_def(v); continue; } @@ -2163,12 +2181,10 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]); } - if (modifiers & HLSL_STORAGE_STATIC) + if (var->storage_modifiers & HLSL_STORAGE_STATIC) list_move_tail(&ctx->static_initializers.instrs, v->initializer.instrs); else list_move_tail(statements_list, v->initializer.instrs); - vkd3d_free(v->initializer.args); - vkd3d_free(v->initializer.instrs); } else if (var->storage_modifiers & HLSL_STORAGE_STATIC) { @@ -2178,32 +2194,33 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t if (type_has_object_components(var->data_type, false)) { - vkd3d_free(v); + free_parse_variable_def(v); continue; } if (!(zero = hlsl_new_uint_constant(ctx, 0, &var->loc))) { - vkd3d_free(v); + free_parse_variable_def(v); continue; } hlsl_block_add_instr(&ctx->static_initializers, zero); if (!(cast = add_cast(ctx, &ctx->static_initializers.instrs, zero, var->data_type, &var->loc))) { - vkd3d_free(v); + free_parse_variable_def(v); continue; } if (!(store = hlsl_new_simple_store(ctx, var, cast))) { - vkd3d_free(v); + free_parse_variable_def(v); continue; } hlsl_block_add_instr(&ctx->static_initializers, store); } - vkd3d_free(v); + free_parse_variable_def(v); } + vkd3d_free(var_list); return statements_list; } @@ -4564,6 +4581,7 @@ preproc_directive: struct_declaration: var_modifiers struct_spec variables_def_optional ';' { + struct parse_variable_def *v, *v_next; struct hlsl_type *type; unsigned int modifiers = $1; @@ -4579,7 +4597,22 @@ struct_declaration: if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) YYABORT; - $$ = declare_vars(ctx, type, modifiers, &@1, $3); + + if ($3) + { + LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) + { + declare_var(ctx, type, modifiers, &@1, v); + } + + if (!($$ = initialize_vars(ctx, $3))) + YYABORT; + } + else + { + if (!($$ = make_empty_list(ctx))) + YYABORT; + } } struct_spec: @@ -5418,12 +5451,20 @@ type_spec: declaration: var_modifiers type variables_def ';' { + struct parse_variable_def *v, *v_next; struct hlsl_type *type; unsigned int modifiers = $1; if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) YYABORT; - $$ = declare_vars(ctx, type, modifiers, &@1, $3); + + LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) + { + declare_var(ctx, type, modifiers, &@1, v); + } + + if (!($$ = initialize_vars(ctx, $3))) + YYABORT; } variables_def_optional: -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
From: Francisco Casas <fcasas(a)codeweavers.com> In a declaration with multiple variables, the variables must be created before the initializer of the next variable is parsed. This is required for initializers such as: float a = 1, b = a, c = b + 1; A requisite for this is that the type information is parsed in the same rule as the first variable (as a variable_def_typed) so it is immediately available to declare the first variable. Then, the next untyped variable declaration is parsed, and the type from the first variable can be used to declare the second, before the third is parsed, and so on. --- libs/vkd3d-shader/hlsl.y | 76 ++++++++++++++++++------ tests/hlsl-initializer-multi.shader_test | 4 +- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 96f3c8b52..a8ec40c3f 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -73,6 +73,10 @@ struct parse_variable_def struct hlsl_semantic semantic; struct hlsl_reg_reservation reg_reservation; struct parse_initializer initializer; + + struct hlsl_type *basic_type; + unsigned int modifiers; + struct vkd3d_shader_location modifiers_loc; }; struct parse_function @@ -1924,11 +1928,13 @@ static bool type_has_numeric_components(struct hlsl_type *type) return false; } -static void declare_var(struct hlsl_ctx *ctx, struct hlsl_type *basic_type, - unsigned int modifiers, const struct vkd3d_shader_location *modifiers_loc, struct parse_variable_def *v) +static void declare_var(struct hlsl_ctx *ctx, struct parse_variable_def *v) { + const struct vkd3d_shader_location *modifiers_loc = &v->modifiers_loc; + struct hlsl_type *basic_type = v->basic_type; struct hlsl_ir_function_decl *func; struct hlsl_semantic new_semantic; + uint32_t modifiers = v->modifiers; bool unbounded_res_array = false; unsigned int invalid_modifiers; struct hlsl_ir_var *var; @@ -4448,6 +4454,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %type <list> unary_expr %type <list> variables_def %type <list> variables_def_optional +%type <list> variables_def_typed %token <name> VAR_IDENTIFIER %token <name> NEW_IDENTIFIER @@ -4510,6 +4517,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %type <variable_def> type_spec %type <variable_def> variable_decl %type <variable_def> variable_def +%type <variable_def> variable_def_typed %% @@ -4602,7 +4610,11 @@ struct_declaration: { LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) { - declare_var(ctx, type, modifiers, &@1, v); + v->basic_type = type; + v->modifiers = modifiers; + v->modifiers_loc = @1; + + declare_var(ctx, v); } if (!($$ = initialize_vars(ctx, $3))) @@ -5449,21 +5461,9 @@ type_spec: } declaration: - var_modifiers type variables_def ';' + variables_def_typed ';' { - struct parse_variable_def *v, *v_next; - struct hlsl_type *type; - unsigned int modifiers = $1; - - if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) - YYABORT; - - LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) - { - declare_var(ctx, type, modifiers, &@1, v); - } - - if (!($$ = initialize_vars(ctx, $3))) + if (!($$ = initialize_vars(ctx, $1))) YYABORT; } @@ -5487,6 +5487,33 @@ variables_def: list_add_tail($$, &$3->entry); } +variables_def_typed: + variable_def_typed + { + if (!($$ = make_empty_list(ctx))) + YYABORT; + list_add_head($$, &$1->entry); + + declare_var(ctx, $1); + } + | variables_def_typed ',' variable_def + { + struct parse_variable_def *head_def; + + assert(!list_empty($1)); + head_def = LIST_ENTRY(list_head($1), struct parse_variable_def, entry); + + assert(head_def->basic_type); + $3->basic_type = head_def->basic_type; + $3->modifiers = head_def->modifiers; + $3->modifiers_loc = head_def->modifiers_loc; + + declare_var(ctx, $3); + + $$ = $1; + list_add_tail($$, &$3->entry); + } + variable_decl: any_identifier arrays colon_attribute { @@ -5528,6 +5555,21 @@ variable_def: ctx->in_state_block = 0; } +variable_def_typed: + var_modifiers type variable_def + { + struct hlsl_type *type; + unsigned int modifiers = $1; + + if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) + YYABORT; + + $$ = $3; + $$->basic_type = type; + $$->modifiers = modifiers; + $$->modifiers_loc = @1; + } + arrays: %empty { diff --git a/tests/hlsl-initializer-multi.shader_test b/tests/hlsl-initializer-multi.shader_test index 9c45a5330..73dd9f2ff 100644 --- a/tests/hlsl-initializer-multi.shader_test +++ b/tests/hlsl-initializer-multi.shader_test @@ -1,4 +1,4 @@ -[pixel shader todo] +[pixel shader] float4 main() : sv_target { float a = 2.0, b = a + 1.0, c = b; @@ -7,7 +7,7 @@ float4 main() : sv_target } [test] -todo draw quad +draw quad probe all rgba (2, 3, 3, 0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
From: Francisco Casas <fcasas(a)codeweavers.com> A struct declaration with variables is now absorbed into the 'declaration' rule, like any other variable declaration. A struct declaration without variables is now reduced to the 'struct_declaration_without_vars' rule. They both are reduced to a 'declaration_statement' in the end. --- libs/vkd3d-shader/hlsl.y | 71 +++++++++--------------- tests/hlsl-initializer-multi.shader_test | 8 +-- 2 files changed, 29 insertions(+), 50 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index a8ec40c3f..37845a0da 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -4449,11 +4449,10 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %type <list> shift_expr %type <list> statement %type <list> statement_list -%type <list> struct_declaration +%type <list> struct_declaration_without_vars %type <list> type_specs %type <list> unary_expr %type <list> variables_def -%type <list> variables_def_optional %type <list> variables_def_typed %token <name> VAR_IDENTIFIER @@ -4586,45 +4585,19 @@ preproc_directive: } } -struct_declaration: - var_modifiers struct_spec variables_def_optional ';' +struct_declaration_without_vars: + var_modifiers struct_spec ';' { - struct parse_variable_def *v, *v_next; - struct hlsl_type *type; - unsigned int modifiers = $1; + if (!$2->name) + hlsl_error(ctx, &@2, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, + "Anonymous struct type must declare a variable."); - if (!$3) - { - if (!$2->name) - hlsl_error(ctx, &@2, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, - "Anonymous struct type must declare a variable."); - if (modifiers) - hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Modifiers are not allowed on struct type declarations."); - } + if ($1) + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, + "Modifiers are not allowed on struct type declarations."); - if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) + if (!($$ = make_empty_list(ctx))) YYABORT; - - if ($3) - { - LIST_FOR_EACH_ENTRY_SAFE(v, v_next, $3, struct parse_variable_def, entry) - { - v->basic_type = type; - v->modifiers = modifiers; - v->modifiers_loc = @1; - - declare_var(ctx, v); - } - - if (!($$ = initialize_vars(ctx, $3))) - YYABORT; - } - else - { - if (!($$ = make_empty_list(ctx))) - YYABORT; - } } struct_spec: @@ -5399,7 +5372,7 @@ type: declaration_statement: declaration - | struct_declaration + | struct_declaration_without_vars | typedef { if (!($$ = make_empty_list(ctx))) @@ -5467,13 +5440,6 @@ declaration: YYABORT; } -variables_def_optional: - %empty - { - $$ = NULL; - } - | variables_def - variables_def: variable_def { @@ -5556,7 +5522,20 @@ variable_def: } variable_def_typed: - var_modifiers type variable_def + var_modifiers struct_spec variable_def + { + struct hlsl_type *type; + unsigned int modifiers = $1; + + if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) + YYABORT; + + $$ = $3; + $$->basic_type = type; + $$->modifiers = modifiers; + $$->modifiers_loc = @1; + } + | var_modifiers type variable_def { struct hlsl_type *type; unsigned int modifiers = $1; diff --git a/tests/hlsl-initializer-multi.shader_test b/tests/hlsl-initializer-multi.shader_test index 73dd9f2ff..8f8a31e20 100644 --- a/tests/hlsl-initializer-multi.shader_test +++ b/tests/hlsl-initializer-multi.shader_test @@ -20,7 +20,7 @@ float4 main() : sv_target } -[pixel shader todo] +[pixel shader] float4 main() : sv_target { struct apple { @@ -32,11 +32,11 @@ float4 main() : sv_target } [test] -todo draw quad +draw quad probe all rgba (7.2, 8.0, 7.2, 8.0) -[pixel shader todo] +[pixel shader] float4 main() : sv_target { struct apple { @@ -48,5 +48,5 @@ float4 main() : sv_target } [test] -todo draw quad +draw quad probe all rgba (5.2, 9.0, 5.2, 9.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250
On Wed Jun 28 20:22:50 2023 +0000, Francisco Casas wrote:
changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/250/diffs?diff_id=54350&start_sha=8cf2faf5737cb162c716f01db1514a4c5da049b3#9155b9453b4ec8ea0b9b025dfb55c061bd931610_2028_2029) For now, I just created a `new_semantic` variable for use be used to initialize the new var, since that pattern is currently being used in `add_semantic_var()`.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250#note_37186
3/5 has some memory-management parts which aren't exactly related to what the patch does. I'm not sure if it's worth splitting at this point, though. One problem with it though is that the invalid modifiers check is part of declare_var(), which means it'll be printed multiple times if there are multiple variables. Apart from that I think the series looks good, and simpler than I would have expected. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250#note_37236
On Wed Jun 28 20:34:20 2023 +0000, Francisco Casas wrote:
For now, I just created a `new_semantic` variable for use be used to initialize the new var, since that pattern is currently being used in `add_semantic_var()`. Yeah, in general I find it more natural to stipulate that ownership is not passed to called functions, except for particular cases (e.g., destroyers). So it would be `hlsl_new_var()`'s duty to duplicate the strings. However it's also fine to settle for a different-than-default choice here.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250#note_37297
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
list_add_tail($$, &$3->entry); }
+variables_def_typed:
I'm not a fan of how the first declaration is somehow "treated differently" than all the others (there is, in some sense, some duplicated code), but there's no need to reshuffle the code for that. Once the problem mentioned by Zeb is fixed, the MR is good for me. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/250#note_37305
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura (＠zfigura)