Skip to content
Snippets Groups Projects

vkd3d-shader: Store shader IR instructions in an array.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:merge_shader_phases into master
1 unresolved thread

Storing in an array facilitates normalisation passes on the IR, and also avoids needing to store complex state information in a future DXIL parser.

Edited by Conor McCarthy

Merge request reports

Merge request pipeline #6701 skipped

Merge request pipeline skipped for d14f42be

Approved by

Merged by Alexandre JulliardAlexandre Julliard 2 years ago (Jan 24, 2023 9:28pm UTC)

Merge details

  • Changes merged into master with d14f42be.
  • 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
  • Conor McCarthy added 1 commit

    added 1 commit

    • cc9a3702 - vkd3d-shader: Merge all shader fork and join phases into a single phase.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • c5032973 - vkd3d-shader: Merge all shader fork and join phases into a single phase.

    Compare with previous version

  • Broadly this seems in the right direction. Patch 1/2 should be split though; for example:

    • Parse d3dbc bytecode up-front into a shader instruction array. This could keep the existing vkd3d_shader_parser interface, and just iterate over the instruction in the array in shader_sm1_read_instruction(). shader_sm1_reset() and shader_sm1_is_end() would have obvious implementations as well.
    • Do the same for vkd3d_shader_sm4_parser.
    • Use the instruction array interface in vkd3d_dxbc_binary_to_text()
    • Likewise for scan_with_parser().
    • The GLSL backend.
    • The SPIR-V backend.

    That's 6 separate parts right there; it seems plausible that the vkd3d_shader_sm1_parser and vkd3d_shader_sm4_parser parts could be further split, but I haven't looked that close yet.

    +        if (!shader_instruction_array_reserve(instructions, 1))
    +        {
    +            ...
    +        }
    +        ins = &instructions->elements[instructions->count];

    shader_instruction_array_reserve() seems a bit awkward, in part because of the similarity in name to vkd3d_array_reserve(), which gets passed the total size to reserve. It might be nicer to do something like "shader_instruction_array_reserve(instructions, instructions->count + 1)".

    +struct vkd3d_shader_instruction_array
    +{
    +    struct vkd3d_shader_instruction *elements;
    +    size_t capacity;
    +    unsigned int count;
    +
    +    struct vkd3d_shader_param_allocator src_params;
    +    struct vkd3d_shader_param_allocator dst_params;
    +};

    It seems odd to use different types for "capacity" and "count" above. Could we use size_t for both?

    Patch 2/2 is a "vkd3d-shader/spirv" patch. I'd probably reframe it as doing the existing transformation on the vkd3d_shader_instruction IR instead of during translation to SPIR-V.

    -    while (!vkd3d_shader_parser_is_end(parser))
    -    {
    -        vkd3d_shader_parser_read_instruction(parser, &instruction);
    -
    -        if (instruction.handler_idx == VKD3DSIH_INVALID)
    -        {
    -            WARN("Encountered unrecognized or invalid instruction.\n");
    -            ret = VKD3D_ERROR_INVALID_SHADER;
    -            break;
    -        }
    -
    -        if ((ret = spirv_compiler_handle_instruction(spirv_compiler, &instruction)) < 0)
    -            break;
    -    }
    -
    -    if (parser->failed)
    -        ret = VKD3D_ERROR_INVALID_SHADER;
    +    ret = spirv_compiler_handle_instructions(spirv_compiler, &parser->instructions);
     
         if (ret >= 0)
             ret = spirv_compiler_generate_spirv(spirv_compiler, compile_info, out);

    It's somewhat tangential to this series, but I think we should pass "parser" to spirv_compiler_generate_spirv(), and then just fold spirv_compiler_handle_instructions() into spirv_compiler_generate_spirv(), analogous to vkd3d_glsl_generator_generate().

  • Conor McCarthy added 9 commits

    added 9 commits

    • 7bc85e0d - vkd3d-shader/sm1: Store parsed instructions in an array.
    • 473a5bea - vkd3d-shader/sm4: Store parsed instructions in an array.
    • 84248e18 - vkd3d-shader: Initialise the instruction array in vkd3d_shader_parser_init().
    • 7cb78117 - vkd3d-shader/trace: Use the instruction array interface in vkd3d_dxbc_binary_to_text().
    • 8d117927 - vkd3d-shader: Use the instruction array interface in scan_with_parser().
    • ce6279d0 - vkd3d-shader/glsl: Use the instruction array interface in vkd3d_glsl_generator_generate().
    • 25f1ebe7 - vkd3d-shader/sm4: Use the instruction array interface in compile_dxbc_tpf().
    • 5858b0ed - vkd3d-shader/spirv: Pass a parser pointer to spirv_compiler_generate_spirv().
    • 9ce5b1a7 - vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • fd00a93c - vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.

    Compare with previous version

  • I did a fairly detailed review of the first two patches in this series, and found a couple of mostly minor issues. I also did a more cursory review of the subsequent 6 patches, and at first sight those seem fine. With the issues fixed, the first 8 patches in this series are probably ready to go in, although I'd like either @giomasce or @zfigura to approve those as well.

    I haven't looked at the last patch in the series in detail yet; it would probably make sense to submit the first 8 patches as a separate MR after fixing the issues.

    +static struct vkd3d_shader_param_node *shader_param_allocator_node_create(
    +        struct vkd3d_shader_param_allocator *allocator)
    +{
    +    struct vkd3d_shader_param_node *node;
    +
    +    if (!(node = vkd3d_malloc(sizeof(*node) + allocator->count * allocator->stride)))
    +        return NULL;
    +    node->next = NULL;
    +    return node;
    +}

    It doesn't bother me too much, but we'd normally write that allocation as:

    vkd3d_malloc(offsetof(struct vkd3d_shader_param_node,
            param[allocator->count * allocator->stride]))
    +static bool shader_param_allocator_init(struct vkd3d_shader_param_allocator *allocator,
    +        unsigned int count, unsigned int stride)
    +{
    +    allocator->count = max(count, 4);
    +    allocator->stride = stride;
    +    allocator->head = shader_param_allocator_node_create(allocator);
    +    allocator->current = allocator->head;
    +    allocator->index = 0;
    +
    +    return true;
    +}

    The return value of shader_param_allocator_node_create() is unchecked above.

    I think there's an argument for just passing the size to shader_param_allocator_node_create(). I.e.:

    count = max(count, 4);
    allocator->count = count;
    ...
    allocator->head = shader_param_allocator_node_create(count * stride);

    mostly because that would avoid passing around a pointer to "allocator" while it's still being constructed. That does slightly complicate shader_param_allocator_get() though, so perhaps the choice is not completely straightforward.

    +void *shader_param_allocator_get(struct vkd3d_shader_param_allocator *allocator, unsigned int count)
    +{
    +    void *params;
    +
    +    if (!count)
    +        return NULL;
    +
    +    if (allocator->index + count > allocator->count)
    +    {
    +        struct vkd3d_shader_param_node *next = shader_param_allocator_node_create(allocator);
    +        allocator->current->next = next;
    +        allocator->current = next;
    +        allocator->index = 0;
    +    }
    +
    +    params = &allocator->current->param[allocator->index * allocator->stride];
    +    allocator->index += count;
    +    return params;
    +}

    The expression "allocator->index + count" can overflow. Rewriting the condition as "if (count > allocator->count - allocator->index)" avoids that issue.

    The return value of shader_param_allocator_node_create() is unchecked.

    +bool shader_instruction_array_init(struct vkd3d_shader_instruction_array *instructions, unsigned int reserve)
    +{
    +    memset(instructions, 0, sizeof(*instructions));
    +    /* Size the parameter initial allocations so they are large enough for most shaders. */
    +    return shader_instruction_array_reserve(instructions, reserve)
    +            && shader_param_allocator_init(&instructions->dst_params, reserve - reserve / 8u, sizeof(*instructions->elements->dst))
    +            && shader_param_allocator_init(&instructions->src_params, reserve * 2u, sizeof(*instructions->elements->src));
    +}

    This would leak "instructions->elements" and "instructions->dst_params.head" if the second shader_param_allocator_init() call were to fail.

    +void shader_instruction_array_destroy(struct vkd3d_shader_instruction_array *instructions, bool destroy_instructions)
    +{
    +    unsigned int i;
    +
    +    if (destroy_instructions)
    +    {
    +        for (i = 0; i < instructions->count; ++i)
    +            if (instructions->elements[i].handler_idx == VKD3DSIH_DCL_IMMEDIATE_CONSTANT_BUFFER)
    +                vkd3d_free((void *)instructions->elements[i].declaration.icb);
    +    }
    +    vkd3d_free(instructions->elements);
    +    shader_param_allocator_destroy(&instructions->dst_params);
    +    shader_param_allocator_destroy(&instructions->src_params);
    +}

    The "destroy_instructions" parameter is not used until much later in the series. I also think it's a little ugly; it may be preferable to just copy "icb" in shader_instruction_array_clone_instruction(). Alternatively, we could store a flag in the vkd3d_shader_instruction structure to indicate whether we have a "cloned" instruction or not.

    Note that with the current scheme there's also a potential issue with reusing the original "icb" storage for VKD3DSIH_DCL_IMMEDIATE_CONSTANT_BUFFER instructions: creating new VKD3DSIH_DCL_IMMEDIATE_CONSTANT_BUFFER instructions in the normaliser would require new allocations for "icb", which would then be leaked by shader_instruction_array_destroy().

    @@ -206,7 +200,8 @@ static bool shader_sm4_read_register_space(struct vkd3d_shader_sm4_parser *priv,
     static void shader_sm4_read_conditional_op(struct vkd3d_shader_instruction *ins, uint32_t opcode,
             uint32_t opcode_token, const uint32_t *tokens, unsigned int token_count, struct vkd3d_shader_sm4_parser *priv)
     {
    -    shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_UINT, &priv->src_param[0]);
    +    shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_UINT,
    +            (struct vkd3d_shader_src_param *)&ins->src[0]);
         ins->flags = (opcode_token & VKD3D_SM4_CONDITIONAL_NZ) ?
                 VKD3D_SHADER_CONDITIONAL_OP_NZ : VKD3D_SHADER_CONDITIONAL_OP_Z;
     }

    Do we need that cast?

    @@ -227,16 +223,23 @@ static void shader_sm4_read_shader_data(struct vkd3d_shader_instruction *ins, ui
    ...
    +    if (!(icb = vkd3d_malloc(sizeof(*icb) + sizeof(*tokens) * icb_size)))

    Like further above, we'd typically use offsetof to calculate the allocation size here.

    @@ -438,8 +441,9 @@ static void shader_sm4_read_dcl_global_flags(struct vkd3d_shader_instruction *in
     static void shader_sm5_read_fcall(struct vkd3d_shader_instruction *ins, uint32_t opcode, uint32_t opcode_token,
             const uint32_t *tokens, unsigned int token_count, struct vkd3d_shader_sm4_parser *priv)
     {
    -    priv->src_param[0].reg.u.fp_body_idx = *tokens++;
    -    shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_OPAQUE, &priv->src_param[0]);
    +    struct vkd3d_shader_src_param *src_params = (struct vkd3d_shader_src_param *)ins->src;
    +    src_params[0].reg.u.fp_body_idx = *tokens++;
    +    shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_OPAQUE, &src_params[0]);
     }
     
     static void shader_sm5_read_dcl_function_body(struct vkd3d_shader_instruction *ins, uint32_t opcode,

    Like above, do we need this cast?

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 7bc85e0d
1648 allocator->current->next = next;
1649 allocator->current = next;
1650 allocator->index = 0;
1651 }
1652
1653 params = &allocator->current->param[allocator->index * allocator->stride];
1654 allocator->index += count;
1655 return params;
1656 }
1657
1658 bool shader_instruction_array_init(struct vkd3d_shader_instruction_array *instructions, unsigned int reserve)
1659 {
1660 memset(instructions, 0, sizeof(*instructions));
1661 /* Size the parameter initial allocations so they are large enough for most shaders. */
1662 return shader_instruction_array_reserve(instructions, reserve)
1663 && shader_param_allocator_init(&instructions->dst_params, reserve - reserve / 8u, sizeof(*instructions->elements->dst))
  • Conor McCarthy added 8 commits

    added 8 commits

    • 9f9f2282 - vkd3d-shader/sm1: Store parsed instructions in an array.
    • 93616a37 - vkd3d-shader/sm4: Store parsed instructions in an array.
    • bab03825 - vkd3d-shader: Initialise the instruction array in vkd3d_shader_parser_init().
    • 92cfeec1 - vkd3d-shader/trace: Use the instruction array interface in vkd3d_dxbc_binary_to_text().
    • a7649670 - vkd3d-shader: Use the instruction array interface in scan_with_parser().
    • 971fe615 - vkd3d-shader/glsl: Use the instruction array interface in vkd3d_glsl_generator_generate().
    • e47c6c7a - vkd3d-shader/sm4: Use the instruction array interface in compile_dxbc_tpf().
    • 3bab3879 - vkd3d-shader/spirv: Pass a parser pointer to spirv_compiler_generate_spirv().

    Compare with previous version

  • Conor McCarthy changed title from vkd3d-shader: Merge all shader fork and join phases into a single phase. to vkd3d-shader: Store shader IR instructions in an array.

    changed title from vkd3d-shader: Merge all shader fork and join phases into a single phase. to vkd3d-shader: Store shader IR instructions in an array.

  • Conor McCarthy changed the description

    changed the description

  • Henri Verbeet approved this merge request

    approved this merge request

  • FTR, I had a look at those patches too, and they make sense to me. I wouldn't call that a real review, but the general concept seems sound and I didn't find and clear problem.

  • added 49 commits

    • 3bab3879...6b82ba94 - 41 commits from branch wine:master
    • 007f894b - vkd3d-shader/sm1: Store parsed instructions in an array.
    • a9aaa59d - vkd3d-shader/sm4: Store parsed instructions in an array.
    • e8cb9060 - vkd3d-shader: Initialise the instruction array in vkd3d_shader_parser_init().
    • e9a2642d - vkd3d-shader/trace: Use the instruction array interface in vkd3d_dxbc_binary_to_text().
    • 2559d622 - vkd3d-shader: Use the instruction array interface in scan_with_parser().
    • 2d3f0518 - vkd3d-shader/glsl: Use the instruction array interface in vkd3d_glsl_generator_generate().
    • 2a5ae0a8 - vkd3d-shader/sm4: Use the instruction array interface in compile_dxbc_tpf().
    • d14f42be - vkd3d-shader/spirv: Pass a parser pointer to spirv_compiler_generate_spirv().

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading