[PATCH 0/1] MR111: vkd3d-shader/hlsl: Improve handling of "technique" tokens.
This change only implements case-sensitivity of technique keywords and related checks when corresponding profile is used. My immediate plan is to add some synthetic type for technique variables, for example as {object, void}, then add named techniques as variables. This is useful because these names are participating in global scope, and should not collide with normal variables. After that "pass_list" will be split, at least in two variants because of how much d3d9 syntax differs. Some trivial changes will be need later to have some top level fx compilation helper that considers only fx objects and calls ps/vs/gs compiler to create inner shader blobs on same original source. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111
From: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.c | 3 ++ libs/vkd3d-shader/hlsl.l | 21 +++++++++++-- libs/vkd3d-shader/hlsl.y | 68 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 88 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index d15abe50..64a6c4df 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -2846,6 +2846,9 @@ static void declare_predefined_types(struct hlsl_ctx *ctx) {"TEXTURE", HLSL_CLASS_OBJECT, HLSL_TYPE_TEXTURE, 1, 1}, {"PIXELSHADER", HLSL_CLASS_OBJECT, HLSL_TYPE_PIXELSHADER, 1, 1}, {"VERTEXSHADER", HLSL_CLASS_OBJECT, HLSL_TYPE_VERTEXSHADER, 1, 1}, + {"technique", HLSL_CLASS_OBJECT, HLSL_TYPE_VOID, 1, 1}, + {"technique10", HLSL_CLASS_OBJECT, HLSL_TYPE_VOID, 1, 1}, + {"technique11", HLSL_CLASS_OBJECT, HLSL_TYPE_VOID, 1, 1}, }; for (bt = 0; bt <= HLSL_TYPE_LAST_SCALAR; ++bt) diff --git a/libs/vkd3d-shader/hlsl.l b/libs/vkd3d-shader/hlsl.l index 59f31fcd..b45046a3 100644 --- a/libs/vkd3d-shader/hlsl.l +++ b/libs/vkd3d-shader/hlsl.l @@ -56,6 +56,10 @@ DOUBLESLASHCOMMENT "//"[^\n]* STRING \"[^\"]*\" IDENTIFIER [A-Za-z_][A-Za-z0-9_]* +TECHNIQUE [Tt][Ee][Cc][Hh][Nn][Ii][Qq][Uu][Ee] +TECHNIQUE10 [Tt][Ee][Cc][Hh][Nn][Ii][Qq][Uu][Ee]10 +TECHNIQUE11 [Tt][Ee][Cc][Hh][Nn][Ii][Qq][Uu][Ee]11 + ANY (.) %% @@ -121,8 +125,21 @@ string {return KW_STRING; } struct {return KW_STRUCT; } switch {return KW_SWITCH; } tbuffer {return KW_TBUFFER; } -technique {return KW_TECHNIQUE; } -technique10 {return KW_TECHNIQUE10; } +{TECHNIQUE} { + struct hlsl_ctx *ctx = yyget_extra(yyscanner); + yylval->name = hlsl_strdup(ctx, yytext); + return KW_TECHNIQUE; + } +{TECHNIQUE10} { + struct hlsl_ctx *ctx = yyget_extra(yyscanner); + yylval->name = hlsl_strdup(ctx, yytext); + return KW_TECHNIQUE10; + } +{TECHNIQUE11} { + struct hlsl_ctx *ctx = yyget_extra(yyscanner); + yylval->name = hlsl_strdup(ctx, yytext); + return KW_TECHNIQUE11; + } texture {return KW_TEXTURE; } texture1D {return KW_TEXTURE1D; } Texture1D {return KW_TEXTURE1D; } diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 4b26a179..c59e431e 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -3745,6 +3745,26 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl } } +static bool hlsl_validate_technique_token(struct hlsl_ctx *ctx, const char *token) +{ + if (ctx->profile->type != VKD3D_SHADER_TYPE_EFFECT) + return true; + + switch (ctx->profile->major_version) + { + case 2: + return !strcmp(token, "technique"); + case 4: + return !strcmp(token, "technique10"); + case 5: + return !strcmp(token, "technique11"); + default: + vkd3d_unreachable(); + } + + return false; +} + } %locations @@ -3837,8 +3857,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %token KW_STRUCT %token KW_SWITCH %token KW_TBUFFER -%token KW_TECHNIQUE -%token KW_TECHNIQUE10 %token KW_TEXTURE %token KW_TEXTURE1D %token KW_TEXTURE1DARRAY @@ -3925,6 +3943,9 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %token <name> NEW_IDENTIFIER %token <name> STRING %token <name> TYPE_IDENTIFIER +%token <name> KW_TECHNIQUE +%token <name> KW_TECHNIQUE10 +%token <name> KW_TECHNIQUE11 %type <arrays> arrays @@ -3957,6 +3978,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %type <name> any_identifier %type <name> var_identifier +%type <name> technique_name %type <parameter> parameter @@ -3994,8 +4016,50 @@ hlsl_prog: destroy_instr_list($2); } | hlsl_prog preproc_directive + | hlsl_prog technique | hlsl_prog ';' +technique_name: + %empty + { + $$ = NULL; + } + | var_identifier + +pass_list: + %empty + +/* Exact name matching only happens for effect profiles and only for keywords specific to particular profile. + Other profiles allow insensitive matching. */ +technique9: + KW_TECHNIQUE technique_name '{' pass_list '}' + { + if (!hlsl_validate_technique_token(ctx, $1)) + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_NOT_DEFINED, "Identifier \"%s\" is not defined.", $1); + hlsl_fixme(ctx, &@$, "Unsupported keyword \"%s\".", $1); + } + +technique10: + KW_TECHNIQUE10 technique_name '{' pass_list '}' + { + if (!hlsl_validate_technique_token(ctx, $1)) + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_NOT_DEFINED, "Identifier \"%s\" is not defined.", $1); + hlsl_fixme(ctx, &@$, "Unsupported keyword \"%s\".", $1); + } + +technique11: + KW_TECHNIQUE11 technique_name '{' pass_list '}' + { + if (!hlsl_validate_technique_token(ctx, $1)) + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_NOT_DEFINED, "Identifier \"%s\" is not defined.", $1); + hlsl_fixme(ctx, &@$, "Unsupported keyword \"%s\".", $1); + } + +technique: + technique9 + | technique10 + | technique11 + buffer_declaration: buffer_type any_identifier colon_attribute { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.c:
{"TEXTURE", HLSL_CLASS_OBJECT, HLSL_TYPE_TEXTURE, 1, 1}, {"PIXELSHADER", HLSL_CLASS_OBJECT, HLSL_TYPE_PIXELSHADER, 1, 1}, {"VERTEXSHADER", HLSL_CLASS_OBJECT, HLSL_TYPE_VERTEXSHADER, 1, 1}, + {"technique", HLSL_CLASS_OBJECT, HLSL_TYPE_VOID, 1, 1}, + {"technique10", HLSL_CLASS_OBJECT, HLSL_TYPE_VOID, 1, 1}, + {"technique11", HLSL_CLASS_OBJECT, HLSL_TYPE_VOID, 1, 1},
This doesn't seem relevant to this patch. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25657
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.l:
STRING \"[^\"]*\" IDENTIFIER [A-Za-z_][A-Za-z0-9_]*
+TECHNIQUE [Tt][Ee][Cc][Hh][Nn][Ii][Qq][Uu][Ee] +TECHNIQUE10 [Tt][Ee][Cc][Hh][Nn][Ii][Qq][Uu][Ee]10 +TECHNIQUE11 [Tt][Ee][Cc][Hh][Nn][Ii][Qq][Uu][Ee]11 + I believe you can write (?i:technique) etc., which seems more readable. (I also wouldn't bother separating these into definitions if it's only used once, especially if shortening with the ignore-case flag.)
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25658
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
+static bool hlsl_validate_technique_token(struct hlsl_ctx *ctx, const char *token) +{ + if (ctx->profile->type != VKD3D_SHADER_TYPE_EFFECT) + return true; + + switch (ctx->profile->major_version) + { + case 2: + return !strcmp(token, "technique"); + case 4: + return !strcmp(token, "technique10"); + case 5: + return !strcmp(token, "technique11"); + default: + vkd3d_unreachable(); + } This doesn't match my testing at all. fx_2_0 doesn't allow technique10 or technique11, but it doesn't care about case, and fx_4_0 and fx_5_0 accept anything. Where are you seeing this?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25659
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
+ %empty + { + $$ = NULL; + } + | var_identifier + +pass_list: + %empty + +/* Exact name matching only happens for effect profiles and only for keywords specific to particular profile. + Other profiles allow insensitive matching. */ +technique9: + KW_TECHNIQUE technique_name '{' pass_list '}' + { + if (!hlsl_validate_technique_token(ctx, $1)) + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_NOT_DEFINED, "Identifier \"%s\" is not defined.", $1); We can factor the hlsl_error() into hlsl_validate_technique_token().
Also, I know this is close to the error that native gives, but it's completely misleading and doesn't reflect the actual condition. We should do better. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25660
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
+ { + $$ = NULL; + } + | var_identifier + +pass_list: + %empty + +/* Exact name matching only happens for effect profiles and only for keywords specific to particular profile. + Other profiles allow insensitive matching. */ +technique9: + KW_TECHNIQUE technique_name '{' pass_list '}' + { + if (!hlsl_validate_technique_token(ctx, $1)) + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_NOT_DEFINED, "Identifier \"%s\" is not defined.", $1); + hlsl_fixme(ctx, &@$, "Unsupported keyword \"%s\".", $1); I don't think we want an hlsl_fixme() here. technique blocks are actually perfectly legal and ignored when not compiling effects.
What we really want is an hlsl_fixme() in hlsl_ctx_init() or something if trying to compile effects, period. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25661
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
destroy_instr_list($2); } | hlsl_prog preproc_directive + | hlsl_prog technique | hlsl_prog ';'
+technique_name: + %empty + { + $$ = NULL; + } + | var_identifier
This doesn't match my testing. According to my testing, it should be NEW_IDENTIFIER, or (probably better) any_identifier with a check for already defined variables and types. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25662
We don't have the infrastructure in place to test actual effects profiles, but at least tests for technique blocks in HLSL shaders would be nice, and would catch some of the inconsistencies I found. (We already have tests for state block syntax in HLSL shaders.) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25663
Also, a minor thing, but I would also simply name the commit something like "Parse technique blocks". "Improve handling" is a bit odd since we didn't handle them at all previously. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25666
On Mon Feb 27 22:48:32 2023 +0000, Zebediah Figura wrote:
This doesn't match my testing at all. fx_2_0 doesn't allow technique10 or technique11, but it doesn't care about case, and fx_4_0 and fx_5_0 accept anything. Where are you seeing this? The idea was to check which techniques should be included in the output. But yes, the way I did is broken. fx_2_0 does allow technique10, it will skip it, but technique11 will trigger compiler error and no output. fx_5_0 will simply promote technique10 to technique11. Case matters, for example fx_2_0 will allow exactly "technique10", but its own "technique" could be in any case it seems.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25704
On Mon Feb 27 22:48:32 2023 +0000, Zebediah Figura wrote:
I believe you can write (?i:technique) etc., which seems more readable. (I also wouldn't bother separating these into definitions if it's only used once, especially if shortening with the ignore-case flag.) I read somewhere that "?i:" is some flex extension, if that's allowed then sure.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25705
On Mon Feb 27 22:48:32 2023 +0000, Zebediah Figura wrote:
I don't think we want an hlsl_fixme() here. technique blocks are actually perfectly legal and ignored when not compiling effects. What we really want is an hlsl_fixme() in hlsl_ctx_init() or something if trying to compile effects, period. They are not really ignored. Syntax should still be valid when not compiling as an effect.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25706
I read somewhere that "?i:" is some flex extension, if that's allowed then sure.
Yes, we explicitly require flex and bison instead of generic lex and yacc. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25744
On Mon Feb 27 22:48:32 2023 +0000, Zebediah Figura wrote:
We can factor the hlsl_error() into hlsl_validate_technique_token(). Also, I know this is close to the error that native gives, but it's completely misleading and doesn't reflect the actual condition. We should do better. I don't think it's misleading, it could say "type is not defined", because such blocks are closer to variables than anything else.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25745
They are not really ignored. Syntax should still be valid when not compiling as an effect.
Well, yes, the syntax needs to be valid, but the point is that we shouldn't emit a fixme just because the technique is declared. I.e. if we remove this fixme, then we correctly compile more HLSL shaders than we previously had done. On the other hand, you can have an fx_4_0 effect without any techniques, and we don't even implement writing that. Hence we probably need a hlsl_fixme() for that. We probably also want an hlsl_fixme() for techniques that's gated to only apply to effects targets. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25780
Case matters, for example fx_2_0 will allow exactly "technique10", but its own "technique" could be in any case it seems.
According to my testing, the keyword itself is case-sensitive for fx_2_0. I.e. in that case "Technique10" isn't a keyword either and can be used as a variable or type name. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_25781
I opened another MR !114 for some initial fx tests. I might make sense to improve handling of fx keywords in non-fx targets first, then add writer for empty effect blobs (for fx_4+ at first). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/111#note_26110
participants (4)
-
Henri Verbeet (＠hverbeet) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)