Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Parse and write default values.

5 unresolved threads

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
  • Francisco Casas added 6 commits

    added 6 commits

    • 35856c4a - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
    • abca0b97 - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
    • 3b080b79 - vkd3d-shader/tpf: Write default values.
    • c6d0175d - tests: Text matrix default value initializers.
    • 5af7d197 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
    • 5f638855 - vkd3d-shader/hlsl: Reorder default values for matrices.

    Compare with previous version

  • Author Developer

    So the pipeline was failing because Zeb's reflection tests in hlsl_d3d12.c detected that I forgot to not write default values for constant buffers variables without initializers (nice!). I was always writing zeros when there was not an initializer. So I fixed that.

    • Why lower them after parsing? Can't we reuse (most of) evaluate_static_expression_as_uint()?

      sm1 has default values as well; if we're not going to write them we should at least have a hlsl_fixme().

    • Author Developer

      Why lower them after parsing? Can't we reuse (most of) evaluate_static_expression_as_uint()?

      It is a little more complicated than when evaluating a single uint because it requires breaking the rhs values into separate components (which can be of different types) and it also must have access to the value of static constants.

      For instance:

      static const float4 a = float4(1, 2, 3, 4);
      int4 b = {1, float2(2, a.z), a.w};

      so the same instruction block must be shared between components and must include the static_initializers instructions.

      I could call constant-folding and copy-propagation early at parse time over ctx->static_initializers for every variable to lower the components to constants as soon as possible but that doesn't seem very clean to me.

      Lowering the default values after the regular constant-folding and copy-prop passes allows for the simple implementation of lower_var_default_values() in the current version.

      sm1 has default values as well; if we're not going to write them we should at least have a hlsl_fixme().

      Oh! I didn't realize about this because they don't show up in native's disassembler. Okay, I added it in 7/10.

    • static const float4 a = float4(1, 2, 3, 4);
      int4 b = {1, float2(2, a.z), a.w};

      so the same instruction block must be shared between components and must include the static_initializers instructions.

      Sorry, I don't understand? Why does this mean we need the same instruction block?

      We should just be able to clone it, prepend the cloned static_initializers the same way that evaluate_static_expression_as_uint() already does, and append a hlsl_new_load_component() as necessary.

    • Ah! I get what you mean. Sorry, for some reason it didn't occur to me to clone the block for each component.

    • Somewhat tangentially, the idea of storing and reusing copyprop state (introduced in the context of loop unrolling) would be useful here, to avoid the need to clone static initializers.

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

    added 8 commits

    • b640bfe7 - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
    • 05548da2 - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
    • c3eaa652 - tests: Test default values using reflection.
    • fb8e16f8 - vkd3d-shader/tpf: Write default values for SM4.
    • 20609d2b - vkd3d-shader/d3dbc: Write default values for SM1.
    • e12e7214 - tests: Text matrix default value initializers.
    • faaf5074 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
    • 384ec54f - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.

    Compare with previous version

    • Could you please add hlsl_fixme()'s to write_fx_2_initial_value() and write_fx_4_numeric_variable() when default values are present.

      Another thing I wanted to mention, where would we put a value pointer for string variables?

    • Could you please add hlsl_fixme()'s to write_fx_2_initial_value() and write_fx_4_numeric_variable() when default values are present.

      done!

      Another thing I wanted to mention, where would we put a value pointer for string variables?

      My guess is that it would make sense to add a const char * field to struct hlsl_default_value. Because of this suggestion I didn't turn hlsl_ir_var.default_values into a mere union hlsl_constant_value_component, because in the future we might want to add extra stuff such as this.

    • Another thing that will be needed is CompileShader/ConstructGSWithSO/compile settings, it's at most 3 strings I think, or 2 strings and a function prototype. We'll probably need small structure for that in hlsl_default_value as well. When used without variable it still creates unnamed object in effects binary, so it's not the same as other state assignments (assignment will be created too, but it will assign that unnamed object).

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

    added 14 commits

    • 384ec54f...afadfe5a - 4 earlier commits
    • 3a17bf27 - tests: Test complex array size expression.
    • 59c18786 - vkd3d-shader/hlsl: Also lower matrix swizzles and index loads in const passes.
    • c5cfb53e - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
    • 582db423 - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
    • e6fad279 - tests: Test default values using reflection.
    • d7ef580a - vkd3d-shader/tpf: Write default values for SM4.
    • b3593ac8 - vkd3d-shader/d3dbc: Write default values for SM1.
    • 9e10606e - tests: Text matrix default value initializers.
    • 86afe650 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
    • 3434cb48 - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.

    Compare with previous version

  • New version, now computing final default values at parse time as suggested by Zeb.

    I think this is good since we no longer require an hlsl_src in struct hlsl_default_value, but it is a little more complex:

    • Requires more passes on constant evaluation, some of these are useful for array sizes though!
    • Requires allowing stores to synthetic variables, which can arise when storing/loading from components, specially if there are value constructors such as float2x3(1,2,3,4,5,6) in the default value initializers.
    • There is more overhead but I doubt this will be a concern.

    I know the MR is chonky now, if this is a problem, I can send the first 6 patches as a separate MR.

  • Francisco Casas mentioned in merge request !839 (merged)

    mentioned in merge request !839 (merged)

    • From 7/14:

      I thought I said this somewhere, but I don't think hlsl_transfer_var_default_values() needs to be an entire helper, nor does it need to be in hlsl.c. I'd just write it inline. Same with hlsl_init_var_default_values().

      +            case HLSL_IR_STORE:
      +                if (hlsl_ir_store(node)->lhs.var->is_synthetic)
      +                    break;
      +                /* fall-through */

      That's odd. Why are there stores left after DCE?

      "save_default_values" feels a bit odd to me naming-wise, and moreover, why do we need that in two separate blocks? Also, why "empty" the instrs block? [Is that left over from the initial revision?]

      The reflection tests (9/14) are a bit odd in that they test things unrelated to default values, but don't quite integrate with test_reflection() either. I'd just pick one or the other...

      Is 12/14 offering any benefit that's not already in the reflection tests?

      14/14 seems like the wrong way to solve this, and the explanation is a bit awkward—default values aren't special either, they're just in actual byte order, which is different from component order. I'd think the right thing to do would be to implement that mapping in hlsl_type_get_component_offset() [which does require handling matrices specially, yes.]

    • I thought I said this somewhere, but I don't think hlsl_transfer_var_default_values() needs to be an entire helper, nor does it need to be in hlsl.c. I'd just write it inline. Same with hlsl_init_var_default_values().

      done.

      That's odd. Why are there stores left after DCE?

      I am not sure I understand the question. hlsl_run_const_passes() is not called yet and it does not run DCE (?)

      I had to allow stores to synthetic variables because they are needed by complex initializers when there are multi-value parse-time arguments in the initializer, such as constructors or matrix indexing:

      static const float3x2 m = {1, 2, 3, 4, 5, 6};
      float4 a = {float2(1, 2), m[2]};

      "save_default_values" feels a bit odd to me naming-wise, and moreover, why do we need that in two separate blocks?

      I changed it to is_default_values_initializer. It is better?

      Ehm, the first block checks if the variable needs to allocate the var->default_values array. The initializer logic in-between the blocks is required whether we need to store the default values or not. And at the end there comes 3 cases from which save_default_values is one of them, to decide what to do with v->initializer.instrs. But you probably already know that, so maybe you are seeing something that I don't see yet.

      The reflection tests (9/14) are a bit odd in that they test things unrelated to default values, but don't quite integrate with test_reflection() either. I'd just pick one or the other...

      Okay, I removed the checks for BoundResources, buffer_desc.Name, buffer_desc.Size, buffer_desc.uFlags, var_desc.Name, var_desc.StartOffset, var_desc.uFlags, var_desc.StartTexture, var_desc.TextureSize, var_desc.StartSampler, and var_desc.SamplerSize.

      Also, why "empty" the instrs block? [Is that left over from the initial revision?]

      Oh, I see now that the call to free_parse_variable_def(v) later takes care of it, yep, so it is not necessary to clean it.

      Is 12/14 offering any benefit that's not already in the reflection tests?

      I feel that it is good for the completeness of the test file, as a sort of documentation of this anomaly. Also, I think it is good to have tests that run infrequent code paths more in isolation, which can be useful e.g. if valgrind or other tool detects an error there.

      14/14 seems like the wrong way to solve this, and the explanation is a bit awkward—default values aren't special either, they're just in actual byte order, which is different from component order. I'd think the right thing to do would be to implement that mapping in hlsl_type_get_component_offset() [which does require handling matrices specially, yes.]

      I am not sure if it is correct to say that they end up in actual byte order, because that is not the case for row_major matrices in SM4:

      row_major int2x3 m = {1, 2, 3, 4, 5, 6};
      //   row_major int2x3 m;                // Offset:    0 Size:    28
      //      = 0x00000001 0x00000003 0x00000005 0x00000000
      //        0x00000002 0x00000004 0x00000006
      
      float4 main() : sv_target
      {
          return m[0][0];
      }

      unless we call it "byte order if the matrix was column-major".

      The way I see it, there are 3 different orders involved:

      • The order of the initializer arguments (A).
      • The HLSL component indexes (B).
      • The offsets in the default values written in the output binary (C), d3dbc or tpf.

      Here is how default values are stored for a column-major and a row-major matrix both in d3dbc and tpf. The possition of the padding words (zeroes) suggests how the values ended up in the HLSL matrix.

      float2x3 m = {1, 2, 3, 4, 5, 6};
      
          stored in d3dbc:  1 4 0 0 2 5 0 0 3 6 0 0     suggests: [ 1 2 3 ]
                                                                  [ 4 5 6 ]
      
          stored in tpf:    1 2 0 0 3 4 0 0 5 6         suggests: [ 1 3 5 ]
                                                                  [ 2 4 6 ]
      row_major float2x3 m = {1, 2, 3, 4, 5, 6}
      
          stored in d3dbc:  1 2 3 0 4 5 6 0             suggests: [ 1 2 3 ]
                                                                  [ 4 5 6 ]
      
          stored in tpf:    1 3 5 0 2 4 6               suggests: [ 1 3 5 ]
                                                                  [ 2 4 6 ]

      Given the values for the HLSL components, there is a transformation from A -> B that is not trivial at least for SM4.

      My current implementation uses hlsl_reorder_default_values_in_matrices() to perform this A -> B transformation and then the same hlsl_type_get_component_offset() we use for the rest of HLSL to do B -> C.

      If I understand correctly, you propose something along the lines of storing the default values as they come in the initializer in var->default_values[] (order A) and then A -> B -> C in hlsl_type_get_component_offset(), probably adding a bool default_values argument.

      This feels odd to me because the index argument would no longer mean "component index in the HLSL sense", but rather "component index in the initializer sense" and we would lost the possibility of directly taking the default value of an HLSL component by accessing var->default_values[] using the component index, if we ever need it.

      But to be completely sure that the anomaly is indeed in the A -> B part, and that is not that the default values for matrices are just stored in an apparently nonsensical way, I will check how ID3DX11EffectPass::Apply() takes this data and uses it to automatically write the constant buffer.

    • That's odd. Why are there stores left after DCE?

      I am not sure I understand the question. hlsl_run_const_passes() is not called yet and it does not run DCE (?)

      Oops, I keep assuming that DCE is always run.

      Thinking about it a bit more though I guess we don't want it here... since we're probably supposed to reject stores to "unrelated" variables. Annoying but I guess this is probably better.

      "save_default_values" feels a bit odd to me naming-wise, and moreover, why do we need that in two separate blocks?

      I changed it to is_default_values_initializer. It is better?

      Ehm, the first block checks if the variable needs to allocate the var->default_values array. The initializer logic in-between the blocks is required whether we need to store the default values or not. And at the end there comes 3 cases from which save_default_values is one of them, to decide what to do with v->initializer.instrs. But you probably already know that, so maybe you are seeing something that I don't see yet.

      No, I'm just blind, sorry.

      Is 12/14 offering any benefit that's not already in the reflection tests?

      I feel that it is good for the completeness of the test file, as a sort of documentation of this anomaly. Also, I think it is good to have tests that run infrequent code paths more in isolation, which can be useful e.g. if valgrind or other tool detects an error there.

      Why not put that documentation in the reflection tests, though?

      I'm not sure I understand the isolation comment. If necessary one can run the reflection tests and comment out all the other tests in that file?

      14/14 seems like the wrong way to solve this, and the explanation is a bit awkward—default values aren't special either, they're just in actual byte order, which is different from component order. I'd think the right thing to do would be to implement that mapping in hlsl_type_get_component_offset() [which does require handling matrices specially, yes.]

      I am not sure if it is correct to say that they end up in actual byte order, because that is not the case for row_major matrices in SM4:

      Ah, you're right. d3dx9 does explicitly transpose, and presumably d3dx10 does too...

      Still, fundamentally this is a translation problem. Using a "pass", especially to perturb the array in place, seems wrong.

      If I understand correctly, you propose something along the lines of storing the default values as they come in the initializer in var->default_values[] (order A) and then A -> B -> C in hlsl_type_get_component_offset(), probably adding a bool default_values argument.

      This feels odd to me because the index argument would no longer mean "component index in the HLSL sense", but rather "component index in the initializer sense" and we would lost the possibility of directly taking the default value of an HLSL component by accessing var->default_values[] using the component index, if we ever need it.

      Not necessarily, we should store it in whatever order is most convenient (but ideally a backend-neutral one, so not the actual register order). Fundamentally we don't have to use hlsl_type_get_component_offset() per se if that doesn't work, but we should be doing the translation in the same place as we were / would be calling that.

    • Why not put that documentation in the reflection tests, though?

      Reading the individual tests can be more readable than these tests that check for many things at the same time.

      I'm not sure I understand the isolation comment. If necessary one can run the reflection tests and comment out all the other tests in that file?

      I meant that this test is even smaller than the reflection test so the possible causes of the error are more narrow. But if you want I can drop the patch.

      Ah, you're right. d3dx9 does explicitly transpose, and presumably d3dx10 does too...

      Yep, note that changing the reading order is not equivalent to transposing though.

      [ 1 2 3 ]   ->   [ 1 3 5 ]
      [ 4 5 6 ]        [ 2 4 6 ]

      Still, fundamentally this is a translation problem. Using a "pass", especially to perturb the array in place, seems wrong.

      Okay. Now the question that remains is whether the translation problem is from the initializer to the HLSL matrix, or from the HLSL matrix to the default values. I think it is the former case given that the output tpf suggest a structure for the matrix.

      I replaced hlsl_reorder_default_values_in_matrices() with the get_component_index_from_default_initializer_index(), which transforms the index from initializer index to component index.

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

    added 157 commits

    • 3434cb48...766913f9 - 143 commits from branch wine:master
    • 766913f9...31738cce - 4 earlier commits
    • f9ac97c3 - tests: Test complex array size expression.
    • 9faf2843 - vkd3d-shader/hlsl: Also lower matrix swizzles and index loads in const passes.
    • 17e3b5ca - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
    • cd61a4cc - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
    • fbc10729 - tests: Test default values using reflection.
    • 2a1d011c - vkd3d-shader/tpf: Write default values for SM4.
    • 1d03b543 - vkd3d-shader/d3dbc: Write default values for SM1.
    • cd33d390 - tests: Test matrix default value initializers.
    • a2414e2f - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
    • 97ca87aa - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.

    Compare with previous version

  • Francisco Casas added 72 commits

    added 72 commits

    • 97ca87aa...4b3a948e - 58 commits from branch wine:master
    • 4b3a948e...f0361e71 - 4 earlier commits
    • 71a8bd43 - tests: Test complex array size expression.
    • 871b4bec - vkd3d-shader/hlsl: Also lower matrix swizzles and index loads in const passes.
    • 6e3ae88d - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
    • 45ede20e - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
    • 337afa95 - tests: Test default values using reflection.
    • 44412af5 - vkd3d-shader/tpf: Write default values for SM4.
    • 856b036b - vkd3d-shader/d3dbc: Write default values for SM1.
    • 17548ada - tests: Test matrix default value initializers.
    • 6ea2cb15 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
    • c8088891 - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.

    Compare with previous version

  • added 1 commit

    • 4a096c2c - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.

    Compare with previous version

  • Francisco Casas added 55 commits

    added 55 commits

    • 4a096c2c...061dc390 - 47 commits from branch wine:master
    • 24869521 - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
    • 5b5cf657 - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
    • 060bf3d4 - tests: Test default values using reflection.
    • 466ca28c - vkd3d-shader/tpf: Write default values for SM4.
    • 2e4f61a8 - vkd3d-shader/d3dbc: Write default values for SM1.
    • bd726113 - tests: Test matrix default value initializers.
    • a78f9399 - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
    • 9173fe34 - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.

    Compare with previous version

  • Francisco Casas added 16 commits

    added 16 commits

    • 9173fe34...9c83caed - 8 commits from branch wine:master
    • 94bfd90c - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
    • 798fbd3d - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
    • 91d55f34 - tests: Test default values using reflection.
    • 9c46f69e - vkd3d-shader/tpf: Write default values for SM4.
    • 91fc5e1f - vkd3d-shader/d3dbc: Write default values for SM1.
    • 6373cdc4 - tests: Test matrix default value initializers.
    • e1255ccb - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
    • 3e9196bc - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 38 commits

    added 38 commits

    • 3e9196bc...1fe7a658 - 30 commits from branch wine:master
    • e8dbc36b - vkd3d-shader/hlsl: Record default values for uniforms and constant buffers.
    • 099a64ae - vkd3d-shader/hlsl: Initialize default values with braceless initializers.
    • b44a25ce - tests: Test default values using reflection.
    • 253c9941 - vkd3d-shader/tpf: Write default values for SM4.
    • affadf31 - vkd3d-shader/d3dbc: Write default values for SM1.
    • ade3daa3 - tests: Test matrix default value initializers.
    • ab01fedc - vkd3d-shader/hlsl: Fix numeric register offset for matrix components.
    • f5bfa728 - vkd3d-shader/hlsl: Reorder default values for matrices for SM4.

    Compare with previous version

Please register or sign in to reply
Loading