Skip to content
Snippets Groups Projects

vkd3d-shader: Normalise signatures and input/output registers to the Shader Model 6 pattern.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:normalise_io into master
5 unresolved threads

Merge request reports

Merge request pipeline #10751 skipped

Merge request pipeline skipped for 6835e817

Approved by

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (May 24, 2023 8:33pm UTC)

Merge details

  • Changes merged into master with 6835e817.
  • 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
  • Author Developer

    Unfortunately I see no reasonable way to split the last patch. It's all interdependent.

    • I haven't quite made it through all the changes in patch 4/4, but some initial comments:

      From patch 1/4:

      +void vkd3d_shader_sm6_free_shader_signature(struct vkd3d_shader_sm6_signature *signature)
      +{
      +    vkd3d_free(signature->elements);
      +    signature->elements = NULL;
      +}

      If this were part of the public API, this should be called "vkd3d_shader_free_sm6_signature()"; for an internal API, the convention would be "vkd3d_shader_sm6_signature_cleanup()". We (Zeb, mostly) have also been omitting the vkd3d_/vkd3d_shader_ prefix on new (module) internal API. I'm not too bothered by either variant, but if you prefer, you could also go with "struct sm6_signature" and "sm6_signature_cleanup()".

      +static bool shader_signature_from_shader_sm6_signature(struct vkd3d_shader_signature *signature,
      +        const struct vkd3d_shader_sm6_signature *src)
      +{
       [...]
      +    if (!(signature->elements = vkd3d_malloc(signature->element_count * sizeof(*signature->elements))))
      +        return false;

      This works, but only because sizeof(*signature->elements) is smaller than sizeof(*src->elements). That may be more fragile than we care for; we'd normally use vkd3d_calloc() here, like shader_parse_signature() does.

      From patch 2/4:

      +    static const DWORD vs_code[] =
      +    {
      +#if 0
      +        uint3 index;
      +
      +        struct vs_data
      +        {
      +            float4 pos : SV_Position;
      +            float4 color[3] : COLOR;
      +        };
      +
      +        void main(in struct vs_data vs_input, out struct vs_data vs_output)
      +        {
      +            vs_output.pos = vs_input.pos;
      +            vs_output.color[0] = vs_input.color[index.x];
      +            vs_output.color[1] = vs_input.color[index.y];
      +            vs_output.color[2] = vs_input.color[index.z];
      +        }
      +#endif
        [...]
      +    };
      +    static const D3D12_SHADER_BYTECODE vs = {vs_code, sizeof(vs_code)};
      +    static const DWORD ps_code[] =
      +    {
      +#if 0
      +        uint4 index;
      +
      +        struct ps_data
      +        {
      +            float4 pos : SV_Position;
      +            float4 color[3] : COLOR;
      +        };
      +
      +        float4 main(struct ps_data ps_input) : SV_Target
      +        {
      +            return ps_input.color[index.w];
      +        }
      +#endif
        [..]
      +    };

      Perhaps the answer is simply "no", but is there any chance the HLSL compiler is already (or soon...) able to compile these and we could use something like hlsl_d3d12.c's "compile_shader" for these?

      From patch 3/4:

      +        /* The normaliser requires the highest slot free in some cases. */
      +        switch (param->type)
      +        {
      +            case VKD3DSPR_INPUT:
      +            case VKD3DSPR_OUTPUT:
      +            case VKD3DSPR_COLOROUT:
      +            case VKD3DSPR_INCONTROLPOINT:
      +            case VKD3DSPR_OUTCONTROLPOINT:
      +            case VKD3DSPR_PATCHCONST:
      +                vkd3d_shader_parser_error(&priv->p, VKD3D_SHADER_ERROR_TPF_PARAM_ORDER_TOO_HIGH,
      +                        "Unexpected I/O parameter order greater than 2.\n");
      +                break;
      +        }

      It's not introduced by this patch, but unfortunately I only notice this issue now: It seems that during the vkd3d_shader_instruction_array conversion a number of checks for "parser->failed" ended up getting dropped. In a lot of cases we also set VKD3DSIH_INVALID, and some of the remaining checks end up catching that and mitigating the issue, but we should be checking for that flag in vkd3d_shader_sm4_parser_create() and vkd3d_shader_sm1_parser_create().

      Both the comment and the error message are perhaps slightly inaccurate; from the point of view of the parser, the issue here is that (depending on the shader type and potentially the phase) shader registers have a particular dimension. Addressing them using either more or fewer indices would be invalid. Perhaps we don't immediately need to be quite that strict (although I suppose we could extend register_type_table[] to include the required information), but I think the error should essentially be something along the lines of "invalid index count for register type".

      From patch 4/4:

      +static void VKD3D_PRINTF_FUNC(3, 4) shader_normaliser_error(struct vkd3d_shader_normaliser *normaliser,
      +        enum vkd3d_shader_error error, const char *format, ...)
      +{
      +    struct vkd3d_shader_location location = {NULL, normaliser->source_index + 1, 0};
      +    va_list args;
      +
      +    va_start(args, format);
      +    vkd3d_shader_verror(normaliser->message_context, &location, error, format, args);
      +    va_end(args);
      +    normaliser->failed = true;
      +}

      That drops the original "source_name" field. We should be able to store that in the vkd3d_shader_instruction_array structure, but we could also just store a vkd3d_shader_location structure inside the vkd3d_shader_instruction structure, which the normaliser could then point to. This can all be tested with vkd3d-compiler and an appropriate shader, of course.

      +static unsigned int shader_signature_find_element_for_reg(const struct vkd3d_shader_sm6_signature *signature,
      +        unsigned int reg_idx, unsigned int write_mask, struct vkd3d_shader_normaliser *normaliser)
      +{
      +    unsigned int signature_idx;
      +
      +    for (signature_idx = 0; signature_idx < signature->element_count; ++signature_idx)
      +    {
       [...]
      +    }
      +
      +    shader_normaliser_error(normaliser, VKD3D_SHADER_ERROR_IR_INVALID_IO_REGISTER,
      +            "Could not find signature element matching register %u and write mask %#x.\n", reg_idx, write_mask);
      +    return ~0u;
      +}

      Technically we're iterating over signature elements above, not signatures; that makes "signature_idx" perhaps a bit awkward as a variable name. We could also just use something like "i", of course.

      I think we could return a pointer to the element itself here, instead of the index. (Like vkd3d_shader_find_signature_element(), incidentally.) All the current callers seem to just use the index to get at the element.

      Somewhat similarly, it probably makes sense to call shader_normaliser_error() from the callers instead of here. On the one hand, all the callers already do some error handling anyway, while on the other hand I could imagine callers potentially being able to give better error messages than we can here.

      +static int signature_element_mask_compare(const void *a, const void *b)
      +{
      +    const struct vkd3d_shader_sm6_signature_element *e = a, *f = b;
      +
      +    if (e->mask == f->mask)
      +        return vkd3d_u32_compare(e->register_index, f->register_index);
      +    return vkd3d_u32_compare(e->mask, f->mask);
      +}

      The code above works, but it is a bit unusual. The usual way to write that would be:

      if ((ret = vkd3d_u32_compare(e->mask, f->mask)))
          return ret;
      return vkd3d_u32_compare(e->register_index, f->register_index);
      +static bool merge_sysval_semantics(struct vkd3d_shader_sm6_signature_element *e,
      +        struct vkd3d_shader_sm6_signature_element *f)
      +{
       [...]
      +}

      Despite the name, this function does not merge anything. Also, the parameters could be const.

      Unfortunately I see no reasonable way to split the last patch. It's all interdependent.

      Introducing "source_index" seem reasonably separate, at least.

    • Author Developer

      This works, but only because sizeof(*signature->elements) is smaller than sizeof(*src->elements).

      I changed it to vkd3d_calloc(), but I see no size mismatch here.

      I think we could return a pointer to the element itself here, instead of the index.

      The index is used to address dcl_params, and it's stored in a register as an index.

    • Please register or sign in to reply
  • We (Zeb, mostly) have also been omitting the vkd3d_/vkd3d_shader_ prefix on new (module) internal API. I'm not too bothered by either variant, but if you prefer, you could also go with "struct sm6_signature" and "sm6_signature_cleanup()".

    My reasoning is that firstly, I like a clear distinction between what is and isn't external API, and secondly, typing out vkd3d_shader everywhere makes names annoyingly long.

    I've said this a few times, but along the same lines, I'd like to propose giving struct vkd3d_shader_instruction (and related structures / functions) a different, shorter prefix in place of "vkd3d_shader". Some ideas:

    • "ir" (to match the file name. Granted, this isn't the only IR in vkd3d-shader, but it's also probably the only one that will have a generic role in every transformation),
    • "vsir" (an abbreviation of "vkd3d-shader IR"),
    • "vazir" (a more cute/clever name, Persian for "minister" but which I understand can also mean something close to "helper" or "assistant", which seems quite fitting. I've tried to also make it into an acronym but the 'z' makes things difficult).
  • Perhaps the answer is simply "no", but is there any chance the HLSL compiler is already (or soon...) able to compile these and we could use something like hlsl_d3d12.c's "compile_shader" for these?

    We need to handle array semantics (148, which I've signed off on and is hopefully just waiting for Gio's sign-off) and then relative addressing (which isn't done but looks pretty easy). I'm of the opinion it shouldn't block this patch, though.

    FWIW, given the simplicity of the test it should even be possible to port it to a shader_test file :-)

  • Conor McCarthy added 11 commits

    added 11 commits

    • 02c19a64...af4bb037 - 7 commits from branch wine:master
    • 988bcceb - vkd3d-shader: Introduce an internal sm6 signature structure.
    • 51ee0594 - tests/d3d12: Test register relative addressing in vertex and pixel shaders.
    • 63b52236 - vkd3d-shader/tpf: Fail parsing if an input/output parameter order is > 2.
    • 8fc936e1 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Conor McCarthy added 6 commits

    added 6 commits

    • 8e42b7c3 - vkd3d-shader/d3dbc: Return an error from vkd3d_shader_sm1_parser_create() if the parser failed.
    • 7d4c2008 - vkd3d-shader/tpf: Return an error from vkd3d_shader_sm4_parser_create() if the parser failed.
    • 35c119cd - vkd3d-shader: Introduce an internal sm6 signature structure.
    • 8b25e7c5 - tests/d3d12: Test register relative addressing in vertex and pixel shaders.
    • 84a6cea5 - vkd3d-shader/tpf: Fail parsing if an input/output parameter order is > 2.
    • f64eb9d0 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • 1dcd92fc - vkd3d-shader: Add source information to normaliser errors and warnings.

    Compare with previous version

  • Conor McCarthy added 2 commits

    added 2 commits

    • 45c3ad29 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
    • 3474d726 - vkd3d-shader: Add source information to normaliser errors and warnings.

    Compare with previous version

    • This works, but only because sizeof(*signature->elements) is smaller than sizeof(*src->elements).

      I changed it to vkd3d_calloc(), but I see no size mismatch here.

      struct sm6_signature_element has the extra "register_count" field that struct vkd3d_shader_signature_element doesn't have; that was the point of this patch, of course. So if "element_count * sizeof(struct sm6_signature_element)" didn't overflow, "element_count * sizeof(struct vkd3d_shader_signature_element) wouldn't either. Though I gather from your reply that that was not the reasoning behind using vkd3d_malloc(). :)

      I think the first 5 patches in the series are fine, and could go in as a separate series.

      Patch 7/7 doesn't touch spirv.c and glsl.c, and I think it should. I.e., spirv_compiler_generate_spirv() and vkd3d_glsl_generator_generate() would ideally use the instruction's "source_index" to set the location. Those parts could do go in independently of the changes in patch 6/7.

      Some comments on patch 6/7:

      • Most (all?) of the VKD3D_SHADER_ERROR_IR_* errors seem like things that the frontend could/should catch. Doing them in the frontend would also allow introducing that validation as separate patches. We could possibly introduce a separate validation pass for that in tpf.c, but I don't think we strictly have to.

      • There's a fair amount of shared state introduced to struct vkd3d_shader_normaliser, but most of it seems local to a particular transformation pass. It probably doesn't need to be part of the vkd3d_shader_normaliser structure, and it probably doesn't even need to be visible outside of ir.c. To some extent that's true for some of the existing shared state in struct vkd3d_shader_normaliser as well.

      • I think we should add the different signatures to struct vkd3d_shader_instruction_array instead of struct vkd3d_shader_normaliser. It would probably simplify passing around the signatures in a couple of other places as well. (In particular, places that currently either use struct vkd3d_shader_desc or pass signatures around as separate parameters.)

      • I don't think the use of the ternary operator in signature_element_mask_compare() improves readability.

      • Somewhat similarly, the "if ((array_size = (input->register_count > 1) ? input->register_count : 0))" line in spirv_compiler_emit_default_control_point_phase() seems a bit too clever. I'd suggest doing something like the following:

          if ((array_size = input->register_count) > 1)
              type_id = vkd3d_spirv_get_op_type_array(...);
          else
              array_size = 0;
      • The patch introduces "outer_array_length" parameters to spirv_compiler_emit_array_variable() and spirv_compiler_emit_builtin_variable(), but the conventional way we'd do that would be to make "array_length"/"array_size" and array and add something like an "index_count"/"length_count"/"size_count" parameter. For example, like we do with vkd3d_spirv_build_op_access_chain() and vkd3d_spirv_build_op_access_chain1(). These changes could be split out as a separate patch. (E.g. "Support emitting multi-dimensional array variables in spirv_compiler_emit_array_variable().")

      • I'm tempted to suggest adding an "index_count" field to struct vkd3d_shader_register. (As opposed to comparing the "offset" field of register indices with ~0u.)

    • Author Developer

      The original used sizeof(*signature->elements) to allocate signature->elements, not sizeof(*src->elements), but if everyone is happy with it now then no worries :)

    • Please register or sign in to reply
  • Henri Verbeet mentioned in merge request !197 (merged)

    mentioned in merge request !197 (merged)

  • Conor McCarthy added 56 commits

    added 56 commits

    • 3474d726...8b57a612 - 48 commits from branch wine:master
    • 655cd577 - vkd3d-shader/tpf: Validate input/output register index counts.
    • 6ce8687c - vkd3d-shader/tpf: Validate signature element register indices.
    • 94ca6552 - vkd3d-shader/tpf: Validate signature element masks.
    • 469de967 - vkd3d-shader/tpf: Validate input/output registers.
    • 0787554e - vkd3d-shader/tpf: Validate index range declarations.
    • 8b4d359f - vkd3d-shader/tpf: Remove an unnecessary carriage return from a parser error message.
    • f043e656 - vkd3d-shader/tpf: Validate input and output index ranges for default control point phases.
    • 8c9bd96c - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Conor McCarthy added 2 commits

    added 2 commits

    • 523c3ce7 - vkd3d-shader/tpf: Emit an error if an index range is declared for default...
    • 585da156 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Conor McCarthy added 10 commits

    added 10 commits

    • f03cd243 - vkd3d-shader/tpf: Validate input/output register index counts.
    • 542b3d77 - vkd3d-shader/tpf: Validate signature element register indices.
    • 7d5357e8 - vkd3d-shader/tpf: Validate signature element masks.
    • 98bec075 - vkd3d-shader/tpf: Validate input/output registers.
    • e6b3ca0b - vkd3d-shader/tpf: Validate index range declarations.
    • aea7fe8f - vkd3d-shader/tpf: Remove an unnecessary carriage return from a parser error message.
    • 8f9cbe03 - vkd3d-shader/tpf: Validate input and output index ranges for default control point phases.
    • 97496f8b - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
    • 85914131 - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
    • 6dabb0b8 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Conor McCarthy added 40 commits

    added 40 commits

    • 6dabb0b8...dc414449 - 37 commits from branch wine:master
    • 0b1e5997 - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
    • 39f2bd4f - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
    • d8db226b - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • 5200a79c - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • 3cabc5b3 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Conor McCarthy added 3 commits

    added 3 commits

    • f8bc003d - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
    • 731bb20b - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
    • 38c9db09 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit f8bc003d
4602 4620 {
4603 4621 if (builtin)
4604 4622 {
4605 input_id = spirv_compiler_emit_builtin_variable(compiler, builtin, storage_class, array_size);
4623 input_id = spirv_compiler_emit_builtin_variable_v(compiler, builtin, storage_class, &array_size, 1);
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit 38c9db09
  • 923 signature = normaliser->output_signature;
    924 break;
    925 default:
    926 return;
    927 }
    928
    929 id_idx = reg->idx_count - 1;
    930 reg_idx = reg->idx[id_idx].offset;
    931 write_mask = VKD3DSP_WRITEMASK_0 << vkd3d_swizzle_get_component(src_param->swizzle, 0);
    932 element_idx = shader_signature_find_element_for_reg(signature, reg_idx, write_mask);
    933
    934 e = &signature->elements[element_idx];
    935 if ((e->register_count > 1 || sysval_semantic_is_tess_factor(e->sysval_semantic)))
    936 id_idx = shader_register_normalise_arrayed_addressing(reg, id_idx, e->register_index);
    937 reg->idx[id_idx].offset = element_idx;
    938 reg->idx_count = id_idx + 1;
    • Comment on lines +937 to +938

      I guess that putting element_idx in the last index slot is intended, to match the code that would be generated by the SM6 parser, isn't it? Because in itself it feels pretty ugly, since it swaps the natural order of array dimensions (i.e., putting major dimensions first).

    • Please register or sign in to reply
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit 38c9db09
  • 978 keep = shader_dst_param_io_normalise(&ins->declaration.register_semantic.reg, true,
    979 normaliser);
    980 break;
    981 case VKD3DSIH_HS_CONTROL_POINT_PHASE:
    982 case VKD3DSIH_HS_FORK_PHASE:
    983 case VKD3DSIH_HS_JOIN_PHASE:
    984 normaliser->phase = ins->handler_idx;
    985 memset(normaliser->input_dcl_params, 0, sizeof(normaliser->input_dcl_params));
    986 memset(normaliser->output_dcl_params, 0, sizeof(normaliser->output_dcl_params));
    987 memset(normaliser->pc_dcl_params, 0, sizeof(normaliser->pc_dcl_params));
    988 break;
    989 default:
    990 if (shader_instruction_is_dcl(ins))
    991 break;
    992 for (i = 0; i < ins->dst_count; ++i)
    993 shader_dst_param_io_normalise((struct vkd3d_shader_dst_param *)&ins->dst[i], false, normaliser);
    • I don't like casting away const too much. What is the reason why dst is a pointer to const in the first pace? Given that it seems that we want to modify instructions, maybe it makes sense to remove const from the struct vkd3d_shader_instruction?

      The same happens a couple of lines below.

    • Please register or sign in to reply
  • One side effect of this MR is that the indices in struct vkd3d_shader_register begin having a different meaning depending on whether they have to be interpreted as SM4-like or SM6-like, which is dictated by the context (right now the context amounts to begin before or after instruction_array_normalise_io_registers(), but in the future it might become more complicated). I think this can be handled, but I don't find ideal. Maybe there should be a flag at some point of the vkd3d_shader_instruction structure tree that should keep track of that information? I don't know, just thinking out loud.

  • Conor McCarthy added 21 commits

    added 21 commits

    • 38c9db09...855bb71f - 18 commits from branch wine:master
    • 756658d8 - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
    • 4a7e7462 - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
    • 29c5e4d5 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Conor McCarthy added 3 commits

    added 3 commits

    • cf4af4eb - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
    • fd6cc23c - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
    • 171747ce - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 5 commits

    • 171747ce...e060773c - 2 commits from branch wine:master
    • 31682c52 - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
    • 110e48e5 - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
    • 6835e817 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading