[PATCH v3 0/5] MR478: vkd3d-shader/hlsl: Parse integers with the 'u' postfix.
-- v3: Revert "vkd3d-shader/hlsl: Do not prioritize an exact match when looking up functions." vkd3d-shader/hlsl: Discern between signed and unsigned ints when parsing. tests: Test overloads with signed and unsigned numeric values. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/hlsl/initializer-numeric.shader_test | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/hlsl/initializer-numeric.shader_test b/tests/hlsl/initializer-numeric.shader_test index 40576887a..2c61f054f 100644 --- a/tests/hlsl/initializer-numeric.shader_test +++ b/tests/hlsl/initializer-numeric.shader_test @@ -46,3 +46,15 @@ float4 main() : sv_target [test] draw quad probe all rgba (10.1, 1.1, 1.2, 2.1) 4 + + +[pixel shader todo] +float4 main() : sv_target +{ + uint4 aa = { 3U, 0xfau, 020u, -1u}; + return float4(aa); +} + +[test] +todo(sm<6) draw quad +todo(sm<6) probe all rgba (3.0, 250.0, 16.0, 4.2949673e+009) 4 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.l | 6 +++--- tests/hlsl/initializer-numeric.shader_test | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.l b/libs/vkd3d-shader/hlsl.l index 0e5f2bb61..b8ce37708 100644 --- a/libs/vkd3d-shader/hlsl.l +++ b/libs/vkd3d-shader/hlsl.l @@ -203,15 +203,15 @@ while {return KW_WHILE; } yylval->floatval = atof(yytext); return C_FLOAT; } -0x[0-9a-fA-F]+ { +0x[0-9a-fA-F]+[uU]? { yylval->intval = vkd3d_parse_integer(yytext); return C_INTEGER; } -0[0-7]+ { +0[0-7]+[uU]? { yylval->intval = vkd3d_parse_integer(yytext); return C_INTEGER; } -[0-9]+ { +[0-9]+[uU]? { yylval->intval = vkd3d_parse_integer(yytext); return C_INTEGER; } diff --git a/tests/hlsl/initializer-numeric.shader_test b/tests/hlsl/initializer-numeric.shader_test index 2c61f054f..087eebf63 100644 --- a/tests/hlsl/initializer-numeric.shader_test +++ b/tests/hlsl/initializer-numeric.shader_test @@ -48,7 +48,7 @@ draw quad probe all rgba (10.1, 1.1, 1.2, 2.1) 4 -[pixel shader todo] +[pixel shader] float4 main() : sv_target { uint4 aa = { 3U, 0xfau, 020u, -1u}; @@ -56,5 +56,5 @@ float4 main() : sv_target } [test] -todo(sm<6) draw quad -todo(sm<6) probe all rgba (3.0, 250.0, 16.0, 4.2949673e+009) 4 +draw quad +probe all rgba (3.0, 250.0, 16.0, 4.2949673e+009) 4 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/hlsl/function-overload.shader_test | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/hlsl/function-overload.shader_test b/tests/hlsl/function-overload.shader_test index ae35c4837..7b298ba30 100644 --- a/tests/hlsl/function-overload.shader_test +++ b/tests/hlsl/function-overload.shader_test @@ -41,6 +41,27 @@ todo(sm<6) draw quad probe all rgba (0.1, 0.2, 0.1, 0.2) +[pixel shader todo fail(sm>=6)] +float func(int arg) +{ + return 1.0; +} + +float func(uint arg) +{ + return 2.0; +} + +float4 main() : sv_target +{ + return float4(func(3u), func(3lu), func(3l), func(3)); +} + +[test] +todo(sm<6) draw quad +probe all rgba (2.0, 2.0, 1.0, 1.0) + + % float and float1 can be defined separately... [pixel shader] -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
On Thu Nov 16 19:08:37 2023 +0000, Nikolay Sivov wrote:
I got a little carried away and checked a few more variants. It also supports "L" and "UL" in all case combinations. Not sure if we need to support that right away, but seems harmless to have it. While "L" part looks meaningless, "U" does for unsigned constant type. You can check that like this: ``` float4 func(int i) { return float4(1, 0, 0, 0); } float4 func(uint i) { return float4(2, 0, 0, 0); }
float4 main() : sv_target { return func(1u); } ``` It's unable to find correct overload with "1" or "1L" though. it is never simple, isn't it? :')
I added a test similar to this one in patch 3/5; patch 4/5 is to identify signed and unsigned numeric values at parse time so we get the data type of the constant right; also, I reverted fef118555cd6341eb648f482a4517d61c347a0d3 in 5/5 so that the test passes. @zfigura I think that the native compiler throwing an error with multiple compatible function overloads with "float" vs "float1" arguments is an exception rather than the rule and we should indeed revert the patch to make more applications work. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52583
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.l | 18 ++++++++++--- libs/vkd3d-shader/hlsl.y | 55 ++++++++++++++++++++++++++++++---------- 2 files changed, 57 insertions(+), 16 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.l b/libs/vkd3d-shader/hlsl.l index b8ce37708..a2dddc99a 100644 --- a/libs/vkd3d-shader/hlsl.l +++ b/libs/vkd3d-shader/hlsl.l @@ -203,18 +203,30 @@ while {return KW_WHILE; } yylval->floatval = atof(yytext); return C_FLOAT; } -0x[0-9a-fA-F]+[uU]? { +0x[0-9a-fA-F]+[lL]? { yylval->intval = vkd3d_parse_integer(yytext); return C_INTEGER; } -0[0-7]+[uU]? { +0[0-7]+[lL]? { yylval->intval = vkd3d_parse_integer(yytext); return C_INTEGER; } -[0-9]+[uU]? { +[0-9]+[lL]? { yylval->intval = vkd3d_parse_integer(yytext); return C_INTEGER; } +0x[0-9a-fA-F]+([uU]|[uU][lL]|[lL][uU]) { + yylval->intval = vkd3d_parse_integer(yytext); + return C_UNSIGNED; + } +0[0-7]+([uU]|[uU][lL]|[lL][uU]) { + yylval->intval = vkd3d_parse_integer(yytext); + return C_UNSIGNED; + } +[0-9]+([uU]|[uU][lL]|[lL][uU]) { + yylval->intval = vkd3d_parse_integer(yytext); + return C_UNSIGNED; + } {WS}+ {} {NEWLINE} { diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 5e0d48d3c..fb6520bc6 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -28,6 +28,12 @@ #define HLSL_YYLTYPE struct vkd3d_shader_location +struct parse_integer +{ + uint32_t u; + bool is_unsigned; +}; + struct parse_fields { struct hlsl_struct_field *fields; @@ -4809,6 +4815,7 @@ static void check_duplicated_switch_cases(struct hlsl_ctx *ctx, const struct hls bool boolval; char *name; DWORD modifiers; + struct parse_integer integer; struct hlsl_ir_node *instr; struct hlsl_block *block; struct list *list; @@ -4939,6 +4946,7 @@ static void check_duplicated_switch_cases(struct hlsl_ctx *ctx, const struct hls %token <floatval> C_FLOAT %token <intval> C_INTEGER +%token <intval> C_UNSIGNED %token <intval> PRE_LINE %type <list> type_specs @@ -4991,6 +4999,7 @@ static void check_duplicated_switch_cases(struct hlsl_ctx *ctx, const struct hls %type <block> unary_expr %type <boolval> boolean +%type <integer> integer %type <buffer_type> buffer_type @@ -5769,7 +5778,7 @@ uav_type: } type_no_void: - KW_VECTOR '<' type ',' C_INTEGER '>' + KW_VECTOR '<' type ',' integer '>' { if ($3->class != HLSL_CLASS_SCALAR) { @@ -5782,21 +5791,21 @@ type_no_void: hlsl_release_string_buffer(ctx, string); YYABORT; } - if ($5 < 1 || $5 > 4) + if ($5.u < 1 || $5.u > 4) { hlsl_error(ctx, &@5, VKD3D_SHADER_ERROR_HLSL_INVALID_SIZE, - "Vector size %d is not between 1 and 4.", $5); + "Vector size %d is not between 1 and 4.", $5.u); YYABORT; } - $$ = hlsl_type_clone(ctx, hlsl_get_vector_type(ctx, $3->base_type, $5), 0, 0); + $$ = hlsl_type_clone(ctx, hlsl_get_vector_type(ctx, $3->base_type, $5.u), 0, 0); $$->is_minimum_precision = $3->is_minimum_precision; } | KW_VECTOR { $$ = hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, 4); } - | KW_MATRIX '<' type ',' C_INTEGER ',' C_INTEGER '>' + | KW_MATRIX '<' type ',' integer ',' integer '>' { if ($3->class != HLSL_CLASS_SCALAR) { @@ -5809,20 +5818,20 @@ type_no_void: hlsl_release_string_buffer(ctx, string); YYABORT; } - if ($5 < 1 || $5 > 4) + if ($5.u < 1 || $5.u > 4) { hlsl_error(ctx, &@5, VKD3D_SHADER_ERROR_HLSL_INVALID_SIZE, - "Matrix row count %d is not between 1 and 4.", $5); + "Matrix row count %d is not between 1 and 4.", $5.u); YYABORT; } - if ($7 < 1 || $7 > 4) + if ($7.u < 1 || $7.u > 4) { hlsl_error(ctx, &@7, VKD3D_SHADER_ERROR_HLSL_INVALID_SIZE, - "Matrix column count %d is not between 1 and 4.", $7); + "Matrix column count %d is not between 1 and 4.", $7.u); YYABORT; } - $$ = hlsl_type_clone(ctx, hlsl_get_matrix_type(ctx, $3->base_type, $7, $5), 0, 0); + $$ = hlsl_type_clone(ctx, hlsl_get_matrix_type(ctx, $3->base_type, $7.u, $5.u), 0, 0); $$->is_minimum_precision = $3->is_minimum_precision; } | KW_MATRIX @@ -6371,6 +6380,18 @@ boolean: $$ = false; } +integer: + C_UNSIGNED + { + $$.u = $1; + $$.is_unsigned = true; + } + | C_INTEGER + { + $$.u = $1; + $$.is_unsigned = false; + } + statement_list: statement | statement_list statement @@ -6664,12 +6685,20 @@ primary_expr: if (!($$ = make_block(ctx, c))) YYABORT; } - | C_INTEGER + | integer { struct hlsl_ir_node *c; - if (!(c = hlsl_new_int_constant(ctx, $1, &@1))) - YYABORT; + if ($1.is_unsigned) + { + if (!(c = hlsl_new_uint_constant(ctx, $1.u, &@1))) + YYABORT; + } + else + { + if (!(c = hlsl_new_int_constant(ctx, $1.u, &@1))) + YYABORT; + } if (!($$ = make_block(ctx, c))) YYABORT; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
From: Francisco Casas <fcasas(a)codeweavers.com> This reverts commit fef118555cd6341eb648f482a4517d61c347a0d3. While it is true that overloads between "float1" and "float" retrieve an error in the native compiler, this is the exception rather than the general rule. There are still many other cases of multiple compatible function overloads, such as the modified test, where the exact match should be prioritized instead of throwing an error, so this is better for applications. --- libs/vkd3d-shader/hlsl.y | 18 ++++++++++++++++++ tests/hlsl/function-overload.shader_test | 4 ++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index fb6520bc6..b8956548d 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2331,6 +2331,21 @@ static struct hlsl_block *initialize_vars(struct hlsl_ctx *ctx, struct list *var return initializers; } +static bool func_is_exact_match(const struct hlsl_ir_function_decl *decl, const struct parse_initializer *args) +{ + unsigned int i; + + if (decl->parameters.count != args->args_count) + return false; + + for (i = 0; i < decl->parameters.count; ++i) + { + if (!hlsl_types_are_equal(decl->parameters.vars[i]->data_type, args->args[i]->data_type)) + return false; + } + return true; +} + static bool func_is_compatible_match(struct hlsl_ctx *ctx, const struct hlsl_ir_function_decl *decl, const struct parse_initializer *args) { @@ -2361,6 +2376,9 @@ static struct hlsl_ir_function_decl *find_function_call(struct hlsl_ctx *ctx, LIST_FOR_EACH_ENTRY(decl, &func->overloads, struct hlsl_ir_function_decl, entry) { + if (func_is_exact_match(decl, args)) + return decl; + if (func_is_compatible_match(ctx, decl, args)) { if (compatible_match) diff --git a/tests/hlsl/function-overload.shader_test b/tests/hlsl/function-overload.shader_test index 7b298ba30..121edf348 100644 --- a/tests/hlsl/function-overload.shader_test +++ b/tests/hlsl/function-overload.shader_test @@ -41,7 +41,7 @@ todo(sm<6) draw quad probe all rgba (0.1, 0.2, 0.1, 0.2) -[pixel shader todo fail(sm>=6)] +[pixel shader fail(sm>=6)] float func(int arg) { return 1.0; @@ -58,7 +58,7 @@ float4 main() : sv_target } [test] -todo(sm<6) draw quad +draw quad probe all rgba (2.0, 2.0, 1.0, 1.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
Seems that this also needs !474 for the pipeline not to fail. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52585
Giovanni Mascellani (@giomasce) commented about tests/hlsl/initializer-numeric.shader_test:
[test] draw quad probe all rgba (10.1, 1.1, 1.2, 2.1) 4 + + +[pixel shader todo] +float4 main() : sv_target +{ + uint4 aa = { 3U, 0xfau, 020u, -1u}; I think this test is slightly more powerful if `aa` has type `float4` instead of `uint4`, so that it is checked that `-1u` is indeed parsed as an unsigned expression (because if it's not the cast to `float` will give `-1` rather than `UINT_MAX`).
Also, I'd test for a number which is in range for `uint` but not for `int`, like three billions. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52665
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
LIST_FOR_EACH_ENTRY(decl, &func->overloads, struct hlsl_ir_function_decl, entry) { + if (func_is_exact_match(decl, args)) + return decl;
I don't think I agree with this. I'd reject valid programs rather than miscompile invalid programs. I think it makes sense to leave your test as `todo` for now, if you (understandably) don't want to work out all the overloading rules for this. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52666
On Thu Nov 16 19:08:37 2023 +0000, Francisco Casas wrote:
it is never simple, isn't it? :') I added a test similar to this one in patch 3/5; patch 4/5 is to identify signed and unsigned numeric values at parse time so we get the data type of the constant right; also, I reverted fef118555cd6341eb648f482a4517d61c347a0d3 in 5/5 so that the test passes. @zfigura I think that the native compiler throwing an error with multiple compatible function overloads with "float" vs "float1" arguments is an exception rather than the rule and we should indeed revert the patch to make more applications work. Do you have applications that are fixed by your revert?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52667
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)