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) March 22, 2024

March 22, 2024

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. > + /* A list containing the state block on the variable's declaration, if any. > + * An array variable may contain multiple state blocks. > * These are only really used for effect profiles. */ > - struct hlsl_state_block *state_block; > + struct list state_blocks; Do we really want this to be a list? I feel like an array is probably going to be nicer to work with... -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65851

1 0

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

March 22, 2024

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h: > HLSL_IR_STORE, > HLSL_IR_SWIZZLE, > HLSL_IR_SWITCH, > + HLSL_IR_UNDECLARED_LOAD, I don't like this name. It's not a load, since it's not coming from a variable. I would rather say something like "effect constant" or "stateblock constant". -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65850

1 0

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

March 22, 2024

Zebediah Figura (@zfigura) commented about tests/hlsl/fx-syntax.shader_test: > +float3 vec; > + > +float4 main() : sv_target { return 0; } > + > +DepthStencilState dss1 > +{ > + RandomField = vec.w; > +}; > + > + > +% Test function call syntax for state blocks. Unlike assignment syntax, only these names are allowed. > +% The parameter count is also checked. > +[pixel shader todo] > +sampler sam > +{ > + SetBlendState(foo, bar, baz); // 3 parameters Can you mix function calls and assignments? I'd assume so but it'd be trivial to test. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65848

1 0

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

March 22, 2024

Zebediah Figura (@zfigura) commented about tests/hlsl/fx-syntax.shader_test: > + pass > + { > + cat = SetPixelShader(foobar); > + } > +} > + > + > +% Test use of a DepthStencilState in SetDepthStencilState(). > +[pixel shader todo] > +DepthStencilState dss1 > +{ > + DepthEnable = false; > + DepthWriteMask = Zero; > + DepthFunc = Less; > + foobar_Field = 22; > +}; This test is a bit weird, because as we've seen, SetDepthStencilState() doesn't even validate anything [unless we're in an effect target, presumably]. Is this actually testing anything? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/739#note_65849

1 0

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

March 22, 2024

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) March 22, 2024

March 22, 2024

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) March 22, 2024

March 22, 2024

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) March 22, 2024

March 22, 2024

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) March 22, 2024

March 22, 2024

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) March 22, 2024

March 22, 2024

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