vkd3d-shader/hlsl: Parse state blocks, part 1.
I am still working on parsing the remaining features of the effects framework, such as the FX functions for the state block entries -- such as SetBlendState() -- and the "compile" and "Compileshader()" syntax. However, after adding the many tests included in 2/7 and reading the feedback from !708 (closed), I think that this first batch of patches are going in the right direction in terms of parsing the state blocks and how to represent them internally.
As Nikolay mentioned in !708 (comment 64421) and !708 (comment 64444) there are many types of state blocks entries, which should be identified for writing the effect metadata.
A part that may cause discussion on this series is that I kept the representation of struct hlsl_state_block_entry
using a hlsl_block
to keep it general enough to represent all these types of state block entries, thinking on later implementing a helper to identify which type of entry we are dealing with.
Even though, as Nikolay pointed out, the instruction set of fx shaders is different, I still think that HLSL IR should be enough to represent the rhs of state blocks, and hopefully we won't need to pollute it too much (apart from the introduction of hlsl_ir_undeclared_load in 4/7 to avoid creating a new variable) if we find operations that are fx-specific, since I intend to represent calls to FX functions with the same struct hlsl_state_block_entry
, given that they cannot be called in regular HLSL code. There are many validations that are applied on regular HLSL that still should be applied to state blocks, such as the use of valid swizzles and the correct use of operators.
Merge request reports
Activity
mentioned in merge request !708 (closed)
458 462 uint32_t is_separated_resource : 1; 459 463 }; 460 464 465 /* This struct is used to represent the two main types of state block entries: 466 * Assignment: 467 * name = {args[0], args[1], ...}; 468 * - or - 469 * name = args[0] 470 * - or - 471 * name[lhs_array_size] = args[0] 472 * - or - 473 * name[lhs_array_size] = {args[0], args[1], ...}; changed this line in version 3 of the diff
added 6 commits
- 63176a88 - tests: Add more tests for fx syntax.
- 5356e04b - vkd3d-shader/hlsl: Parse and store state blocks on variables.
- 883a85e2 - vkd3d-shader/hlsl: Introduce hlsl_ir_undeclared_load.
- 09c24618 - vkd3d-shader/hlsl: Parse list of state blocks.
- 326cde33 - vkd3d-shader/hlsl: Store state block on pass variables.
- f8115253 - vkd3d-shader/hlsl: Allow KW_PIXELSHADER and KW_VERTEXSHADER as stateblock lhs.
Toggle commit listWe are currently lowering all dereferences into a single offset node and a regset (which is used to discern when variables belong to multiple regsets). Copy propagation also relies on this.
Dereferences to pixel shaders and vertex shaders are created on the instructions originated from prepend_uniform_copy(), when the original uniform is copied into the temp.
It can happen merely by declaring PixelShader or VertexShader variables
PixelShader ps1; float4 main() : sv_target { return 0; }
That makes sense!
I implemented it but there is a detail: the current implementation of compute_liveness_recurse() (required by dce) only works with offset nodes. So I extended it to work with both paths and offsets. I don't think it looks bad with the introduction of the deref_mark_last_read() helper.
We may eventually remove the rel_offset parts when we complete the translation to vsir, because we would be able to use remove_dead_code() there.
I implemented it but there is a detail: the current implementation of compute_liveness_recurse() (required by dce) only works with offset nodes. So I extended it to work with both paths and offsets. I don't think it looks bad with the introduction of the deref_mark_last_read() helper.
Yeah, that sounds right.
484 /* Whether the lhs in an assignment is an array and, in that case, its size. */ 485 bool lhs_is_array; 486 unsigned int lhs_array_size; 487 488 /* For assignments, instructions present in the rhs. For function calls, instructions present 489 * in the function arguments. */ 490 struct hlsl_block *instrs; 491 492 /* For assignments, arguments of the rhs initializer. For function calls, the function 493 * arguments. */ 494 struct hlsl_ir_node **args; 495 unsigned int args_count; 496 497 /* For assignments, whether the rhs is wrapped in braces or not. */ 498 bool rhs_braces; 499 }; - Comment on lines +477 to +499
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.
well, first we have to find out if native does some compilation passes such as copy-propagation and constant folding to lower complicated expressions into constants, and apply those passes. For instance, are rhs expressions such as
ANISOTROPIC + 1
marked as a of the constant load type in the effects metadata?After applying the passes we have to check the type of the last instruction of the block (or args[0], in case ths is not a complex initializer with braces) which represents the whole expression, and see if it is HLSL_IR_CONSTANT, HLSL_IR_INDEX (in which case we have to check if the index part is constant) or HLSL_IR_EXPR.
We should also probably introduce a pass exclusive to fx.c to lower known HLSL_IR_UNDECLARED_LOAD into constants, so constant folding (if required) can work.
I don't know yet how complex initializers with braces work in this case, if at all, but they parse.
They are evaluated, yes. Destination index is not evaluated, it has to be an integer literal. So full format would be like this:
property_name[] = ( literal_constant | variable[literal_constant] | variable[index_variable] | )
Expression case does not map directly to our internal opcodes, it does not follow the same conventions as d3dbc/tpf binaries do. But we can leave that out for now.
Another case that does not fit into "args" array is examples like "compile ps_2_0, main(var)", or asm {} blocks, or asm{} blocks with leading decl{} part. Those will have to be handled separately.
Another case that does not fit into "args" array is examples like "compile ps_2_0, main(var)", or asm {} blocks, or asm{} blocks with leading decl{} part. Those will have to be handled separately.
The approach I am writing for the
compile ps_2_0 main(var)
compile expressions is creating a HLSL_IR_COMPILE_SHADER node type. I think this is necessary because these can also appear outside state blocks (which means they are part of regular HLSL syntax):PixelShader ps1 = compile ps_2_0 main(); technique { pass { SetPixelShader(ps1); } }
so it would be a matter of checking if rhs's arg[0] is of this type of node.
I haven't hear of asm{} blocks before, but I assume something similar would happen.
471 * name[lhs_array_size] = args[0] 472 * - or - 473 * name[lhs_array_size] = {args[0], args[1], ...}; 474 * FX function call: 475 * name(args[0], args[1], ...); 476 */ 477 struct hlsl_state_block_entry 478 { 479 bool is_function_call; 480 481 /* For assignments, the name in the lhs. For function calls, the name of the function. */ 482 char *name; 483 484 /* Whether the lhs in an assignment is an array and, in that case, its size. */ 485 bool lhs_is_array; 486 unsigned int lhs_array_size; - Comment on lines +484 to +486
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-variable-syntax
changed this line in version 3 of the diff
Interesting, when compiling the following shader with with native and
fx_4_0
:BlendState myBS { BlendEnable = false; }; technique { }
I get:
blendenable1.hlsl(3,5-24): Index is required for state 'BlendEnable'
meaning that it is an index, and for some state members it is required at least for
fx_4_0
, it is not forfx_2_0
.Didn't your test confirm that it doesn't? For this particular field, it's a change in d3d10.1 that added per-target values. But anyway, this does not concern the parsing part at all, we'll emit errors only for fx profiles. For regular profiles validity of field name or index bounds are not validated. Regarding has_index, we probably need something like that unfortunately, because index limits are [0,4294967295]. We could still use -1 as special value, because actual limits will be lower, but it will be less readable I suspect.
460 464 465 /* This struct is used to represent the two main types of state block entries: 466 * Assignment: 467 * name = {args[0], args[1], ...}; 468 * - or - 469 * name = args[0] 470 * - or - 471 * name[lhs_array_size] = args[0] 472 * - or - 473 * name[lhs_array_size] = {args[0], args[1], ...}; 474 * FX function call: 475 * name(args[0], args[1], ...); 476 */ 477 struct hlsl_state_block_entry 478 { 479 bool is_function_call; changed this line in version 3 of the diff
This is especially dead code if we aren't even parsing functions yet.
Okay, I removed it for now. I also removed
rhs_braces
following that policy.And... do we really want to do it this way? Could we store them in a separate array?
I think it is convenient given the similarities of both formats. Also, I think it was suggested in the other MR.
The complication of having two struts, as I see it, can arise from having to define
struct hlsl_state_block_entry
as the union of both structs with some flag to identify which one of these we are dealing with. This, because both types can be contained in a state block. That, or having them in two different arrays instruct hlsl_state_block
.Edited by Francisco Casas
5549 5567 %type <variable_def> variable_def 5550 5568 %type <variable_def> variable_def_typed 5551 5569 5570 %type <state_block> state_block 5571 changed this line in version 3 of the diff
5322 5339 struct parse_attribute_list attr_list; 5323 5340 struct hlsl_ir_switch_case *switch_case; 5324 5341 struct hlsl_scope *scope; 5342 struct hlsl_state_block *state_block; Only for consistency with
hlsl_ir_var.state_block
(and thenhlsl_ir_var.state_blocks
).When I have to copy a struct that contains pointers I prefer to do it by copying the pointer than doing a copy-on-assignment (or having to specify the fields to be copied individually), but it is just personal preference. If you want I can change it.
423 423 /* Scope that contains annotations for this variable. */ 424 424 struct hlsl_scope *annotations; 425 425 426 /* The state block on the variable's declaration, if any. 427 * These are only really used for effect profiles. */ 428 struct hlsl_state_block *state_block; Well, as I mentioned in the other MR, I have a preference for having the structs that own other structs through pointers, as pointers themselves. For instance, if we put this struct in an array (as in the new version of that patch) without doing it through pointers, a reallocation will invalidate previous pointers to them. Granted, this doesn't happen right now, and probably will never happen for this struct, but I don't think there is any downside either? I can change it to a flat struct if you prefer.
6693 6706 6694 6707 state_block: 6695 6708 %empty 6696 | state_block state 6709 { 6710 if (!($$ = hlsl_alloc(ctx, sizeof(*$$)))) 6711 YYABORT; 6712 } 6713 | state_block any_identifier '[' C_INTEGER ']' '=' complex_initializer ';' changed this line in version 3 of the diff
- tests/hlsl/fx-syntax.shader_test 0 → 100644
85 } 86 }, 87 { 88 { 89 Filter = ANISOTROPIC; 90 }, 91 { 92 Filter = ANISOTROPIC; 93 } 94 } 95 }; 96 97 float4 main() : sv_target { return 0; } 98 99 100 % Arrays of size 1 can still use a single state block without the need to put it inside a list. changed this line in version 3 of the diff
- tests/hlsl/fx-syntax.shader_test 0 → 100644
399 float3 vec; 400 401 float4 main() : sv_target { return 0; } 402 403 DepthStencilState dss1 404 { 405 RandomField = vec.w; 406 }; 407 408 409 % Test function call syntax for state blocks. Unlike assignment syntax, only these names are allowed. 410 % The parameter count is also checked. 411 [pixel shader todo] 412 sampler sam 413 { 414 SetBlendState(foo, bar, baz); // 3 parameters changed this line in version 3 of the diff
There is a test, if this is what you mean:
% It is not allowed to call the functions to set state blocks on the rhs using the assignment syntax % for state groups or passes. [pixel shader fail(sm<6)] float4 main() : sv_target { return 0; } technique { pass { cat = SetPixelShader(foobar); } }
But other types of function calls on the rhs are allowed. I added some more tests.
- tests/hlsl/fx-syntax.shader_test 0 → 100644
461 pass 462 { 463 cat = SetPixelShader(foobar); 464 } 465 } 466 467 468 % Test use of a DepthStencilState in SetDepthStencilState(). 469 [pixel shader todo] 470 DepthStencilState dss1 471 { 472 DepthEnable = false; 473 DepthWriteMask = Zero; 474 DepthFunc = Less; 475 foobar_Field = 22; 476 }; changed this line in version 3 of the diff
299 299 HLSL_IR_STORE, 300 300 HLSL_IR_SWIZZLE, 301 301 HLSL_IR_SWITCH, 302 HLSL_IR_UNDECLARED_LOAD, changed this line in version 3 of the diff
424 424 /* Scope that contains annotations for this variable. */ 425 425 struct hlsl_scope *annotations; 426 426 427 /* The state block on the variable's declaration, if any. 427 /* A list containing the state block on the variable's declaration, if any. 428 * An array variable may contain multiple state blocks. 428 429 * These are only really used for effect profiles. */ 429 struct hlsl_state_block *state_block; 430 struct list state_blocks; changed this line in version 3 of the diff
I changed it to an array of pointers.
The list makes it easier to pass it around across the different parse structs and finally the variable. With arrays the parsing code is a little uglier, for instance, I had to make state_block_list a
struct parse_variable_def
instead of a merestruct list
, but I agree it can make the rest of the code that uses this field simpler.The list makes it easier to pass it around across the different parse structs and finally the variable. With arrays the parsing code is a little uglier, for instance, I had to make state_block_list a
struct parse_variable_def
instead of a merestruct list
, but I agree it can make the rest of the code that uses this field simpler.I find that preferable actually; I don't like passing untyped lists around.
added 10 commits
- 949943fb - vkd3d-shader/hlsl: Also call dce before lowering deref paths.
- a1f7ed19 - tests: Add more state block syntax tests.
- 576e109e - tests: Test function call syntax for state blocks.
- 3980f9c7 - tests: Add tests for fxgroup syntax.
- ab5b3841 - tests: Add tests for "compile" and CompileShader() syntax.
- 6754b497 - vkd3d-shader/hlsl: Parse and store state blocks on variables.
- 9fc89f4b - vkd3d-shader/hlsl: Introduce hlsl_ir_stateblock_constant.
- 1b48e385 - vkd3d-shader/hlsl: Parse list of state blocks.
- 5bda3219 - vkd3d-shader/hlsl: Store state block on pass variables.
- 9c1cea7e - vkd3d-shader/hlsl: Allow KW_PIXELSHADER and KW_VERTEXSHADER as stateblock lhs.
Toggle commit listadded 8 commits
- 765ae51e - tests: Test function call syntax for state blocks.
- 7de5eb11 - tests: Add tests for fxgroup syntax.
- a74b094e - tests: Add tests for "compile" and CompileShader() syntax.
- c0d835d8 - vkd3d-shader/hlsl: Parse and store state blocks on variables.
- 71cda7f2 - vkd3d-shader/hlsl: Introduce hlsl_ir_stateblock_constant.
- c8d49cb4 - vkd3d-shader/hlsl: Parse list of state blocks.
- f1568a4e - vkd3d-shader/hlsl: Store state block on pass variables.
- f37208fa - vkd3d-shader/hlsl: Allow KW_PIXELSHADER and KW_VERTEXSHADER as stateblock lhs.
Toggle commit list