Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Parse ConstructGSWithSO() and minor fixes.

Merged Francisco Casas requested to merge fcasas/vkd3d:stateblock_experiments_4_part2 into master
2 unresolved threads

Merge request reports

Merge request pipeline #31829 skipped

Merge request pipeline skipped for ffc14494

Merged by Henri VerbeetHenri Verbeet 6 months ago (Sep 14, 2024 2:53pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
2785 2794 initialize_var_components(ctx, v->initializer.instrs, var, &store_index, v->initializer.args[k]);
2786 2795 }
2787 2796
2788 if (is_default_values_initializer)
2797 if (var->default_values)
2789 2798 {
2799 /* Default values are discarded if they contain an object
2800 * literal for a numeric component. */
2801
2802 for (k = 0; k < component_count; ++k)
2803 {
2804 if (hlsl_is_numeric_type(hlsl_type_get_component_type(ctx, type, k))
2805 && var->default_values[k].is_object_literal)
  • Comment on lines +2804 to +2805

    Can't we deal with this inside initialize_var_components()? Do we need to store this in the variable?

  • Author Developer

    In principle we don't need to store it in the variable, but in my opinion it is nice to have it in struct hlsl_default_value because that way it can be retrieved as part of the return value in evaluate_static_expression() instead of being passed as an additional parameter.

    Also struct hlsl_default_value already needs to store some information regarding the type of the provided data, albeit implicitly: when hlsl_default_value.string is not NULL it is assumed that the value is a string.

    Removing this field would also mean to add another output parameter to initialize_var_components().

  • In principle we don't need to store it in the variable, but in my opinion it is nice to have it in struct hlsl_default_value because that way it can be retrieved as part of the return value in evaluate_static_expression() instead of being passed as an additional parameter.

    Do we actually need evaluate_static_expression() to deal with that? Can't we just check the node type in initialize_var_components()? If it's not numeric or a string then we blank the variable default_values, emit an error, and abort. I don't see a reason why initialize_vars() actually needs to deal with this.

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

    changed this line in version 2 of the diff

  • Author Developer

    Okay, I had to add a bool is_default_values_initializer parameter to initialize_var_components() though, because we previously checked whether dst->default_values was NULL to know that.

  • Okay, I had to add a bool is_default_values_initializer parameter to initialize_var_components() though, because we previously checked whether dst->default_values was NULL to know that.

    Yeah, that seems fine.

    Actually, initialize_var_components() seems like an anti-helper to me; I would rather just make it two separate functions, one to deal with default values, and the other for writing into a block. But we can leave that to later.

  • Please register or sign in to reply
  • Elizabeth Figura
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit cc0fcca4
  • 874 874 {
    875 875 struct hlsl_ir_node node;
    876 876
    877 /* Special field to store the profile argument. */
    877 enum hlsl_compile_type
    878 {
    879 /* A shader compilation through the CompileShader() function or the "compile" syntax. */
    880 HLSL_COMPILE_TYPE_COMPILE = 0,
    881 } compile_type;
    882
    883 /* Special field to store the profile argument for HLSL_COMPILE_TYPE_COMPILE. */
    • This looks like pretty much all these fields are specific to one or the other. Should this just be a separate node type?

    • Author Developer

      These common features might be enough to consider them the same node type:

      • They have to store their own block of instructions required for the arguments (instrs field) unlike regular hlsl_ir_calls.
      • After parsing, they are only meaningful for effects.
      • They end up resulting in bytecode.

      And adding a new node adds some boilerplate, but I don't have a strong opinion. If you still feel these should be separated, I will do it.

    • Are these the only "compile_types" we will ever see?

      They don't seem very similar, either conceptually or in the code. Perhaps Nikolay would assert otherwise though. Either way I'm not sure it's my place to insist they be separated.

    • Main difference is that ConstructGSWithSO() does not compile, it only attaches extra fields to the initial value and also affects type data in effect binary - GSWithSO and GS are two distinct types there, but that's only an implementation quirk because initializer has more fields. They are similar in a sense that both are "constructors" for GeometryShader type. Actually, we could even rename hlsl_ir_compile to hlsl_ir_shader to reflect that.

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

    added 4 commits

    • fdc92840 - tests: Test ConstructGSWithSO parsing.
    • eb823a25 - vkd3d-shader/hlsl: Process GeometryShader as a valid stateblock lhs.
    • 900541f8 - vkd3d-shader/hlsl: Parse ConstructGSWithSO().
    • 7c4543a9 - vkd3d-shader/hlsl: Allow effect calls on default value initializers.

    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 82 commits

    added 82 commits

    • 7c4543a9...4c03cda3 - 77 commits from branch wine:master
    • 02b249d5 - vkd3d-shader/hlsl: Introduce enum hlsl_compile_type.
    • 3423d1b5 - tests: Test ConstructGSWithSO() parsing.
    • 1f9fc2a4 - vkd3d-shader/hlsl: Process GeometryShader as a valid stateblock lhs.
    • 4aa262d7 - vkd3d-shader/hlsl: Parse ConstructGSWithSO().
    • ffc14494 - vkd3d-shader/hlsl: Allow effect calls on default value initializers.

    Compare with previous version

  • Please register or sign in to reply
    Loading