March 2024 - Wine-gitlab - list.winehq.org

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Zebediah Figura (＠zfigura) 22 Mar '24

22 Mar '24

Zebediah Figura (@zfigura) commented about tests/hlsl/fx-syntax.shader_test: > + } > + }, > + { > + { > + Filter = ANISOTROPIC; > + }, > + { > + Filter = ANISOTROPIC; > + } > + } > +}; > + > +float4 main() : sv_target { return 0; } > + > + > +% Arrays of size 1 can still use a single state block without the need to put it inside a list. There's a lot of tests for valid array usage, but not invalid array usage. What if the count is mismatched? Can an array of size 1 take interior braces? Can a non-array take interior braces? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65847

1 0

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Zebediah Figura (＠zfigura) 22 Mar '24

22 Mar '24

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y: > > state_block: > %empty > - | state_block state > + { > + if (!($$ = hlsl_alloc(ctx, sizeof(*$$)))) > + YYABORT; > + } > + | state_block any_identifier '[' C_INTEGER ']' '=' complex_initializer ';' You could probably avoid the duplication with a separate "%empty | [int]" rule. Or alternatively use "arrays" and add an explicit check for count > 1. Is C_INTEGER correct here, or should we allow compile-time constants? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65846

1 0

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Zebediah Figura (＠zfigura) 22 Mar '24

22 Mar '24

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h: > /* Scope that contains annotations for this variable. */ > struct hlsl_scope *annotations; > > + /* The state block on the variable's declaration, if any. > + * These are only really used for effect profiles. */ > + struct hlsl_state_block *state_block; Does this need to be a pointer? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65845

1 0

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Zebediah Figura (＠zfigura) 22 Mar '24

22 Mar '24

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y: > %type <variable_def> variable_def > %type <variable_def> variable_def_typed > > +%type <state_block> state_block > + This is out of alphabetical order. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65843

1 0

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Zebediah Figura (＠zfigura) 22 Mar '24

22 Mar '24

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y: > struct parse_attribute_list attr_list; > struct hlsl_ir_switch_case *switch_case; > struct hlsl_scope *scope; > + struct hlsl_state_block *state_block; Does this need to be a pointer? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65844

1 0

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Zebediah Figura (＠zfigura) 22 Mar '24

22 Mar '24

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h: > > +/* This struct is used to represent the two main types of state block entries: > + * Assignment: > + * name = {args[0], args[1], ...}; > + * - or - > + * name = args[0] > + * - or - > + * name[lhs_array_size] = args[0] > + * - or - > + * name[lhs_array_size] = {args[0], args[1], ...}; > + * FX function call: > + * name(args[0], args[1], ...); > + */ > +struct hlsl_state_block_entry > +{ > + bool is_function_call; This is especially dead code if we aren't even parsing functions yet. And... do we really want to do it this way? Could we store them in a separate array? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65842

1 0

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Zebediah Figura (＠zfigura) 22 Mar '24

22 Mar '24

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h: > + * name[lhs_array_size] = args[0] > + * - or - > + * name[lhs_array_size] = {args[0], args[1], ...}; > + * FX function call: > + * name(args[0], args[1], ...); > + */ > +struct hlsl_state_block_entry > +{ > + bool is_function_call; > + > + /* For assignments, the name in the lhs. For function calls, the name of the function. */ > + char *name; > + > + /* Whether the lhs in an assignment is an array and, in that case, its size. */ > + bool lhs_is_array; > + unsigned int lhs_array_size; I suspect this is rather an index. Effect documentation is nonexistent, but [1] shows some hints, this is probably how you set per-RT blend states. It's probably worth testing with an actual effect target to see if BlendEnable is different from BlendEnable[0]. [1] https://learn.microsoft.com/en-us/windows/win32/direct3d11/d3d11-effect-var… -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65841

1 0

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Nikolay Sivov (＠nsivov) 22 Mar '24

22 Mar '24

Nikolay Sivov (@nsivov) commented about libs/vkd3d-shader/hlsl.h: > + /* Whether the lhs in an assignment is an array and, in that case, its size. */ > + bool lhs_is_array; > + unsigned int lhs_array_size; > + > + /* For assignments, instructions present in the rhs. For function calls, instructions present > + * in the function arguments. */ > + struct hlsl_block *instrs; > + > + /* For assignments, arguments of the rhs initializer. For function calls, the function > + * arguments. */ > + struct hlsl_ir_node **args; > + unsigned int args_count; > + > + /* For assignments, whether the rhs is wrapped in braces or not. */ > + bool rhs_braces; > +}; My concern, just by looking at it, is about how hard would it be to extract relevant cases from instructions. Is it easy to tell already that we got a constant load, array variable load with constant index, array variable load with variable index, or more complicated expression? Those are important to distinguish, because they are encoded differently. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65840

1 0

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Zebediah Figura (＠zfigura) 22 Mar '24

22 Mar '24

> +% For now, these tests are just to check proper parsing of effects syntax. > +% Most of these tests are useless as effects. This file is very large; I'd propose splitting it into state-block-syntax (which already exists) and something more like effect-state and effect-shaders? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65839

1 0

Re: [PATCH v2 0/7] MR739: vkd3d-shader/hlsl: Parse state blocks, part 1.
by Zebediah Figura (＠zfigura) 22 Mar '24

22 Mar '24

> This is required before introducing the upcoming tests because otherwise > we reach unreacheable code when new_offset_instr_from_deref() calls > type_get_regset() on pixel or vertex shaders. Why are we creating offset instrs for objects? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65838

1 0