Skip to content
Snippets Groups Projects

vkd3d-shader/fx: Add initial support for writing buffers descriptions.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:fx_cb into master
6 unresolved threads

Merge request reports

Checking pipeline status.

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Feb 19, 2024 9:59pm UTC)

Merge details

  • Changes merged into with 315b7c5a.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Nikolay Sivov added 4 commits

    added 4 commits

    • bbcf634e - vkd3d-shader/fx: Do not align strings for fx_4/fx_5 profiles.
    • ec55553c - vkd3d-shader/fx: Mark constant buffers empty on creation.
    • c1694bc2 - vkd3d-shader/fx: Add initial support for writing buffers descriptions.
    • 13f03d03 - vkd3d-shader/fx: Do not align structured data section.

    Compare with previous version

  • Giovanni Mascellani resolved all threads

    resolved all threads

    • +    struct fx_4_numeric_type
      +    {
      +        uint32_t type_class   : 3;
      +        uint32_t base_type    : 5;
      +        uint32_t rows         : 3;
      +        uint32_t columns      : 3;
      +        uint32_t column_major : 1;
      +        uint32_t unknown      : 17;
      +    };
       [...]
      +    union
      +    {
      +        struct fx_4_numeric_type type;
      +        uint32_t data;
      +    } numeric_desc;
       [...]
      +        put_u32_unaligned(buffer, numeric_desc.data);

      I don't think bit-field memory layout is particularly portable.

    • Author Developer

      I see, that's unfortunate. Should I use masks and shifts instead?

      P.S. do you think we need to revert wine@3724385c for the same reason?

      Edited by Nikolay Sivov
    • Please register or sign in to reply
    • I see, that's unfortunate. Should I use masks and shifts instead?

      It seems preferable to me, yes.

      P.S. do you think we need to revert wine@3724385c for the same reason?

      I wouldn't have written it that way, but my opinion on Wine's d3d10/d3dcompiler module isn't necessarily relevant. :) One thing to perhaps point out though is that the code in Wine targets a potentially narrower set of platforms than vkd3d.

    • Author Developer

      Alright, pushed now without using bitfields.

    • Please register or sign in to reply
  • Nikolay Sivov added 2 commits

    added 2 commits

    • c24edbbd - vkd3d-shader/fx: Add initial support for writing buffers descriptions.
    • fd9970d3 - vkd3d-shader/fx: Do not align structured data section.

    Compare with previous version

  • Nikolay Sivov added 2 commits

    added 2 commits

    • fbadf034 - vkd3d-shader/fx: Add initial support for writing buffers descriptions.
    • 45bbc7ae - vkd3d-shader/fx: Do not align structured data section.

    Compare with previous version

    • This is useful for implicit $Globals buffer when writing effects binaries. It should not be written out if there is no global variables, but when there are some, it should appear first in order.

      What if there are global variables, but they're unused? I.e. should this just be checking "used_size"?

      If the answer is no, I think this is still redundant with "size".

    • Author Developer

      Unused ones are included as well. Yes, it makes sense to use 'size', and actually it's necessary to set offsets anyway.

      Edited by Nikolay Sivov
    • Author Developer

      I see now why additional field was easier - 'size' is set later on, that involves extern_vars which is not populated initially, and that depends on uniform_copy() which, if separated, depends on entry point presence. Still, it's better to use the same field everywhere, I'll see how I can separate this in a way that makes sense. E.g. there is no need to handle function parameters for effects, I don't think it's possible to use them there, and have $Params block written out as a result.

    • Author Developer

      I pushed something for this. It does introduce some duplication. To avoid it, _emit() has to change to set buffer "size" and update variable offsets without adding variables.

    • Please register or sign in to reply
  • Elizabeth Figura
  • Nikolay Sivov added 18 commits

    added 18 commits

    • 45bbc7ae...5c917552 - 14 commits from branch wine:master
    • 56e7ca91 - vkd3d-shader/fx: Use variable pointer in write_group().
    • 77d71f72 - vkd3d-shader/fx: Do not align strings for fx_4/fx_5 profiles.
    • 39c5a9e0 - vkd3d-shader/fx: Add initial support for writing buffers descriptions.
    • daa0b45e - vkd3d-shader/fx: Do not align structured data section.

    Compare with previous version

    • Do we really want to be defining a type rbtree like this? The existing code is explicitly set up to be usable by the backend in such a way—cf. the "bytecode_offset" member—and sm4 types (and I think also sm1) are similarly supposed to be deduplicated, we just don't because it doesn't actually matter.

      I'm not sure the answer is "no", mind. There's something to be said for orthogonality, and if we're going to be shoving things through vsir then we should maybe try to avoid hlsl leaking through so much. [Although on the other hand, if we're shoving things through vsir then how do we build reflection data? That's an unsolved question.] But I'm not sure the answer is yes either.

      Either way, do we really want it to be an rbtree? It doesn't seem likely we'll have enough types to matter, and rbtrees are broadly just more work to deal with than an array or list.

    • Author Developer

      I don't see your point. Why not have it as an rbtree? We have it like that for the compiler. There is one compare function, init/destroy, and that's it. Regarding duplicated types, what I want is a matching binary, because it's much easier to compare this way. There should be no problem of getting it to match in metadata parts, shaders will obviously differ.

    • I don't see your point. Why not have it as an rbtree? We have it like that for the compiler. There is one compare function, init/destroy, and that's it.

      I'm not saying it's a lot more work, but it's still a bit more work. You have to define two callbacks, for instance, which you don't have to do with a simpler data structure. I don't think it's wrong to say that any use of a more complex data structure in place of a simpler one deserves justification.

      The compiler uses an rbtree, but well, it was like that when I got here. The compiler also defines a lot more builtin types, and even then I'm not sure it's enough that an rbtree is actually an improvement.

    • Author Developer

      No problem in switching to a list, on my side. For strings it's normally a larger number of elements, because every type name, string constant, and variable name is there, but also not necessarily relevant for performance reasons, until we have actual numbers.

    • Author Developer

      Trees are also used for parameters in d3dx9 effects, and for types in d3d10 ones, so I didn't think twice.

    • Author Developer

      Alright, push a variant with a list. Please take a look.

    • I would say that RB trees are essentially the same amount of work as lists (incidentally, I wish we used a language that made both more ergonomical and type-safe to use, but that's another matter). Given that the implementation is already done in both cases, I don't see strong reasons to prefer one or the other in abstract. Here we need, it seems, an unordered associative container supporting element addition and element retrieval from a key, which maps rather naturally to a RB tree, which is on average more efficient than a linked list for these operations. Of course it might be that in practical cases the list might be approximately as efficient, or even better, than the tree, but in those cases the cost is likely to be very small anyway; and in other (even if somewhat unexpected) cases the tree is likely to be much better than the list. Overall I think I'd choose the RB tree, though I can't say my opinion is really strong here.

    • Please register or sign in to reply
  • Nikolay Sivov added 10 commits

    added 10 commits

    • daa0b45e...5eba031f - 6 commits from branch wine:master
    • a5a65e8b - vkd3d-shader/fx: Use variable pointer in write_group().
    • e12e084f - vkd3d-shader/fx: Do not align strings for fx_4/fx_5 profiles.
    • 7006510f - vkd3d-shader/fx: Add initial support for writing buffers descriptions.
    • 2c27bcfb - vkd3d-shader/fx: Do not align structured data section.

    Compare with previous version

  • Author Developer

    Are there any specific suggestions for me to improve this?

  • Elizabeth Figura
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit 7006510f
  • 653 put_u32(buffer, bind_point); /* Bind point */
    654
    655 put_u32(buffer, 0); /* Annotations count */
    656 /* FIXME: write annotations */
    657
    658 count = 0;
    659 size = 0;
    660 LIST_FOR_EACH_ENTRY(var, &ctx->globals->vars, struct hlsl_ir_var, scope_entry)
    661 {
    662 if (var->buffer != b)
    663 continue;
    664
    665 write_fx_4_variable(var, fx);
    666 size += get_fx_4_type_size(var->data_type);
    667 ++count;
    668 }
    • Comment on lines +660 to +668

      This writes every global variable, including unused, static, and synthetic variables. Is that correct?

    • Author Developer

      There is no synthetic variables at the time this is used. It needs to write every global variable, doesn't matter if it's used or not.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 7006510f
  • 683 {
    684 if (!(var->storage_modifiers & HLSL_STORAGE_UNIFORM) || hlsl_type_is_resource(var->data_type))
    685 continue;
    686
    687 if (hlsl_var_has_buffer_offset_register_reservation(ctx, var))
    688 hlsl_calculate_buffer_offset(ctx, var, true);
    689 }
    690
    691 LIST_FOR_EACH_ENTRY(var, &ctx->globals->vars, struct hlsl_ir_var, scope_entry)
    692 {
    693 if (!(var->storage_modifiers & HLSL_STORAGE_UNIFORM) || hlsl_type_is_resource(var->data_type))
    694 continue;
    695
    696 if (!hlsl_var_has_buffer_offset_register_reservation(ctx, var))
    697 hlsl_calculate_buffer_offset(ctx, var, false);
    698 }
    • Comment on lines +682 to +698

      Duplicating this code is a bit unfortunate. Can we use a helper instead?

      Also, what about the rest of allocate_buffers()? Should we be moving things to $Params? validate_buffer_offsets()? Is buffer binding allocation validated, even if it's not going to be used?

    • Author Developer

      One problem with using allocate_buffers() is that binding validation when multiple buffers are using same register should not be enforced. Regarding $Params, I don't think you can ever get that buffer written when using effects. It's only used for entry point arguments, right? For effects I don't think you can use entry points like that.

    • Makes sense. In that case it'd still be nice to find a way to define a helper, say, hlsl_calculate_buffer_offsets().

    • Nikolay Sivov changed this line in version 7 of the diff

      changed this line in version 7 of the diff

    • Author Developer

      Yes, that makes sense. I pushed a change for that. It's now using extern_vars too.

    • Please register or sign in to reply
  • Sorry, I was still considering the hlsl_type question, and waiting for opinions from others.

    I've given this another review and noticed some more things.

  • Nikolay Sivov added 36 commits

    added 36 commits

    • 2c27bcfb...628acb6b - 32 commits from branch wine:master
    • b88067af - vkd3d-shader/fx: Use variable pointer in write_group().
    • 4ee714a0 - vkd3d-shader/fx: Do not align strings for fx_4/fx_5 profiles.
    • 34d3eb14 - vkd3d-shader/fx: Add initial support for writing buffers descriptions.
    • 4ef1c035 - vkd3d-shader/fx: Do not align structured data section.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading