[PATCH v4 0/4] MR478: vkd3d-shader/hlsl: Parse integers with the 'u' postfix.
-- v4: vkd3d-shader/hlsl: Discern between signed and unsigned ints when parsing. tests: Test overloads with signed and unsigned numeric values. vkd3d-shader/hlsl: Parse integers with the 'u' postfix. tests: Test integers with the 'u' postfix. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/hlsl/initializer-numeric.shader_test | 28 ++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/hlsl/initializer-numeric.shader_test b/tests/hlsl/initializer-numeric.shader_test index 40576887a..d480b9d1d 100644 --- a/tests/hlsl/initializer-numeric.shader_test +++ b/tests/hlsl/initializer-numeric.shader_test @@ -46,3 +46,31 @@ 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 +{ + float4 aa = { 3U, 0xfau, 020u, -1u}; + return aa; +} + +[test] +todo(sm<6) draw quad +todo(sm<6) probe all rgba (3.0, 250.0, 16.0, 4.2949673e+009) 4 + + +[require] +shader model < 6.0 + +[pixel shader todo] +float4 main() : sv_target +{ + // 3000000000 is -1294967296 when it doesn't have the 'u' suffix, except in SM6. + float2 aa = {3000000000, 3000000000U}; + return float4(aa, 0.0, 0.0); +} + +[test] +todo draw quad +todo probe all rgba (-1294967296.0, 3000000000.0, 0.0, 0.0) 4 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
On Fri Nov 17 09:38:09 2023 +0000, Giovanni Mascellani wrote:
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. Good idea, it is indeed as you say.
I also added an additional test for 3 billions. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52739
On Fri Nov 17 09:39:51 2023 +0000, Giovanni Mascellani wrote:
Do you have applications that are fixed by your revert? I don't remember specific applications, but if I remember correctly from when I tried to sort out the function overload orders, the exact matches always have priority when there are multiple compatible function overloads.
Albeit I didn't detect that overloads whose only difference is having `float1` and `float` arguments throw an error. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52738
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.l | 6 +++--- tests/hlsl/initializer-numeric.shader_test | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.l b/libs/vkd3d-shader/hlsl.l index 0e5f2bb61..c35709903 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 d480b9d1d..81d19d06c 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 { float4 aa = { 3U, 0xfau, 020u, -1u}; @@ -56,14 +56,14 @@ float4 main() : sv_target } [test] -todo(sm<6) draw quad +draw quad todo(sm<6) probe all rgba (3.0, 250.0, 16.0, 4.2949673e+009) 4 [require] shader model < 6.0 -[pixel shader todo] +[pixel shader] float4 main() : sv_target { // 3000000000 is -1294967296 when it doesn't have the 'u' suffix, except in SM6. @@ -72,5 +72,5 @@ float4 main() : sv_target } [test] -todo draw quad +draw quad todo probe all rgba (-1294967296.0, 3000000000.0, 0.0, 0.0) 4 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
On Fri Nov 17 09:38:11 2023 +0000, Giovanni Mascellani wrote:
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. Okay, I shouldn't have relied on my memory alone. Also with your suggestion to change the test from `uint4` to `float4` we now have a test that shows the importance of 4/5, so I am okay with dropping 5/5.
I will keep in mind to gather some tests for my assessment that the exact matches always have priority when there are multiple compatible function overloads, or maybe I can be easily proven wrong with a counter example, but that can be left to another MR. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52740
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
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.l | 18 +++++-- libs/vkd3d-shader/hlsl.y | 55 +++++++++++++++++----- tests/hlsl/initializer-numeric.shader_test | 4 +- 3 files changed, 59 insertions(+), 18 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.l b/libs/vkd3d-shader/hlsl.l index c35709903..401fba604 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; } diff --git a/tests/hlsl/initializer-numeric.shader_test b/tests/hlsl/initializer-numeric.shader_test index 81d19d06c..617b67405 100644 --- a/tests/hlsl/initializer-numeric.shader_test +++ b/tests/hlsl/initializer-numeric.shader_test @@ -57,7 +57,7 @@ float4 main() : sv_target [test] draw quad -todo(sm<6) probe all rgba (3.0, 250.0, 16.0, 4.2949673e+009) 4 +probe all rgba (3.0, 250.0, 16.0, 4.2949673e+009) 4 [require] @@ -73,4 +73,4 @@ float4 main() : sv_target [test] draw quad -todo probe all rgba (-1294967296.0, 3000000000.0, 0.0, 0.0) 4 +probe all rgba (-1294967296.0, 3000000000.0, 0.0, 0.0) 4 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478
@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.
It's not so much that native never prioritizes an exact match—certainly there's *some* prioritization logic—but more that it doesn't *always* prioritize an exact match. I.e. the rules are more complicated than that, and, as Giovanni says, I think we'd rather reject valid programs with a hlsl_fixme(), than accept invalid ones. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52781
Why introduce the separate "integer" rule? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52782
Why introduce the separate "integer" rule?
I suppose part of the reason is to handle vector and matrix, but it turns out those are supposed to be able to take arbitrary expressions anyway, constant folding and all. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_52783
On Tue Nov 21 00:11:32 2023 +0000, Zebediah Figura wrote:
@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. It's not so much that native never prioritizes an exact match—certainly there's *some* prioritization logic—but more that it doesn't *always* prioritize an exact match. I.e. the rules are more complicated than that, and, as Giovanni says, I think we'd rather reject valid programs with a hlsl_fixme(), than accept invalid ones. Okay, maybe I don't remember correctly.
I already removed the revert commit though. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/478#note_53000
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Zebediah Figura (＠zfigura)