Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Parse state blocks, part 1.

Merged Francisco Casas requested to merge fcasas/vkd3d:stateblock_experiments_2_part1 into master

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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], ...};
  • 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.

    Compare with previous version

    • 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?

    • We 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; }
    • Shouldn't DCE be killing off those dereferences?

    • DCE happens after offsets are lowered.

    • I would solve that problem by running it again, then, earlier.

    • 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.

    • Please register or sign in to reply
    • +% 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?

    • I spread the tests between state-block-syntax.shader_test, state-block-function-syntax.shader_test, fxgroup-syntax.shader_test, and effect-compile.shader_test.

    • Please register or sign in to reply
  • Nikolay Sivov
    Nikolay Sivov @nsivov started a thread on commit 2e37460a
  • 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.

    • ...which means we would have to somewhat propagate the loads to pixel|vertex shader variables into these HLSL_IR_COMPILE_SHADER. Maybe something worth adding to the copy-propagation pass if we end up using it for rhs.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 5356e04b
  • 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

    • Francisco Casas changed this line in version 3 of the diff

      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 for fx_2_0.

    • I was curious and did a couple more tests, and it turns out that for fx_4_1, "SrcBlend = src_color" will actually emit all 8 indices separately, but "SrcBlend[0] = src_color" just emits index 0. So that's interesting.

    • This is not related to how this should be parsed though.

    • This is not related to how this should be parsed though.

      Not as such. That said, if there were evidence that "SrcBlend[0]" behaved exactly like "SrcBlend", that would imply we don't even need a has_index field.

    • 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.

    • Didn't your test confirm that it doesn't?

      Yes, exactly. I'm just trying to explain why I wanted to test it in the first place. If the results has been otherwise, that would imply a slight change in how we parse things.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 5356e04b
  • 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;
    • 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?

    • Francisco Casas changed this line in version 3 of the diff

      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.

    • I dunno. It seems awkward to me, and it's not clear to me that defining a separate struct is actually going to complicate the code at all?

    • 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 in struct hlsl_state_block.

      Edited by Francisco Casas
    • I'd probably define them as two separate arrays, but in any case I'm not sure I see why that makes things more complicated?

    • Well, just more count and capacity fields and the implicit assumption that the order of the state block entries doesn't matter.

      I think Nikolay is also in favor of keeping them in the same struct; given his reply to my status e-mail.

    • I still don't like overloading fields like that, but I don't really want to argue about this either.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 5356e04b
  • 5549 5567 %type <variable_def> variable_def
    5550 5568 %type <variable_def> variable_def_typed
    5551 5569
    5570 %type <state_block> state_block
    5571
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit 5356e04b
  • 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;
    • Does this need to be a pointer?

    • Only for consistency with hlsl_ir_var.state_block (and then hlsl_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.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 5356e04b
  • 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;
    • Does this need to be a pointer?

    • 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.

    • Well, the downside is we have to do more malloc/free, mainly. I don't know that it's a strong downside, but it is the kind of thing I tend to avoid. I at least can't promise I won't convert it back to a flat struct at some point if we leave it like this.

    • Seems fair to me, if you find a reason to flatten it I won't comply.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 5356e04b
  • 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 ';'
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 63176a88
  • 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.
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 63176a88
  • 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
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 63176a88
  • 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 };
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 883a85e2
  • 299 299 HLSL_IR_STORE,
    300 300 HLSL_IR_SWIZZLE,
    301 301 HLSL_IR_SWITCH,
    302 HLSL_IR_UNDECLARED_LOAD,
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 09c24618
  • 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;
    • Comment on lines -429 to +430

      Do we really want this to be a list? I feel like an array is probably going to be nicer to work with...

    • Francisco Casas changed this line in version 3 of the diff

      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 mere struct 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 mere struct 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.

    • Please register or sign in to reply
  • Francisco Casas added 10 commits

    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.

    Compare with previous version

  • added 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.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading