vkd3d-shader/fx: Add initial support for writing buffers descriptions.
Merge request reports
Activity
- Resolved by Nikolay Sivov
- Resolved by Giovanni Mascellani
- Resolved by Nikolay Sivov
added 4 commits
Toggle commit list+ 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.
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
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.
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".
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 SivovI 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.
- Resolved by Nikolay Sivov
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.
Toggle commit list-
45bbc7ae...5c917552 - 14 commits from branch
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.
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.
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.
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.
Toggle commit list-
daa0b45e...5eba031f - 6 commits from branch
- Resolved by Nikolay Sivov
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 } 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
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.
changed this line in version 7 of the diff
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.
Toggle commit list-
2c27bcfb...628acb6b - 32 commits from branch