Skip to content
Snippets Groups Projects

ci: Validate the SPIR-V code.

Merged Giovanni Mascellani requested to merge giomasce/vkd3d:amiata into master
1 unresolved thread

In order to validate SPIR-V code we have to fix a little bug that causes us to emit invalid SPIR-V code in some cases. That's technically a bug, so it should be good to fix it in 1.10; unfortunately the way the bug is fixed right now is not ideal, because in some cases it causes many push constants to be wasted, potentially reaching the Vulkan implementation limit, so some application that used to work might stop working. Eventually we should implement a better allocator for push constants, and include an alternative path if the Vulkan implementation doesn't offer enough push constants.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • The pipeline failure is expected, because until this MR is accepted the Docker image still misses package spirv-tools.

  • -static void vkd3d_spirv_validate(const struct vkd3d_shader_code *spirv,
    +static bool vkd3d_spirv_validate(struct spirv_compiler *compiler, const struct vkd3d_shader_code *spirv,
             enum vkd3d_shader_spirv_environment environment)
     {
         spv_diagnostic diagnostic = NULL;
         spv_context context;
    -    spv_result_t ret;
    +    spv_result_t res;
    +    bool ret = true;
     
         context = spvContextCreate(spv_target_env_from_vkd3d(environment));
     
    -    if ((ret = spvValidateBinary(context, spirv->code, spirv->size / sizeof(uint32_t),
    +    if ((res = spvValidateBinary(context, spirv->code, spirv->size / sizeof(uint32_t),
                 &diagnostic)))
         {
    -        FIXME("Failed to validate SPIR-V binary, ret %d.\n", ret);
    -        FIXME("Diagnostic message: %s.\n", debugstr_a(diagnostic->error));
    +        FIXME("Failed to validate SPIR-V binary, result %d.\n", res);
    +        TRACE("Diagnostic message: %s.\n", debugstr_a(diagnostic->error));
    +        vkd3d_shader_trace_text(diagnostic->error, strlen(diagnostic->error));
    +
    +        if (compiler->config_flags & VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION)
    +        {
    +            spirv_compiler_error(compiler, VKD3D_SHADER_ERROR_SPV_INVALID_SHADER,
    +                    "Execution generated an invalid shader:\n%s", diagnostic->error);
    +            ret = false;
    +        }
         }
     
         spvDiagnosticDestroy(diagnostic);
         spvContextDestroy(context);
    +
    +    return ret;
     }

    Could we just vkd3d_string_buffer_printf() diagnostic->error here? It would allow the caller to decide what to do with the error, slightly reduce the amount of code inside #ifdef, and probably avoid moving a bunch of code around as well.

  • Can't comment on the last commit for the CI build, but the rest looks good, and I second Henri's comment.

  • added 2 commits

    • b0d3d88b - ci: Build vkd3d with SPIRV-Tools.
    • 0684a976 - vkd3d-shader/spirv: Honor force_validation after emitting SPIR-V code.

    Compare with previous version

  • added 36 commits

    • 0684a976...45679a96 - 32 commits from branch wine:master
    • 2eca62bd - vkd3d: Pad push constant ranges to 16 bytes.
    • 6012cb13 - vkd3d-shader/spirv: Move the SPIR-V dumper and validator below struct spirv_compiler.
    • 0e37d671 - ci: Build vkd3d with SPIRV-Tools.
    • 1696bc9c - vkd3d-shader/spirv: Honor force_validation after emitting SPIR-V code.

    Compare with previous version

  • added 2 commits

    • f724162d - vkd3d-shader/spirv: Honor force_validation after emitting SPIR-V code.
    • 1613c424 - ci: Build vkd3d with SPIRV-Tools.

    Compare with previous version

  • @@ -2448,7 +2451,8 @@ static void spirv_compiler_destroy(struct spirv_compiler *compiler)
     static struct spirv_compiler *spirv_compiler_create(const struct vkd3d_shader_version *shader_version,
             struct vkd3d_shader_desc *shader_desc, const struct vkd3d_shader_compile_info *compile_info,
             const struct vkd3d_shader_scan_descriptor_info1 *scan_descriptor_info,
    -        struct vkd3d_shader_message_context *message_context, const struct vkd3d_shader_location *location)
    +        struct vkd3d_shader_message_context *message_context, const struct vkd3d_shader_location *location,
    +        bool config_flags)
     {
         const struct shader_signature *patch_constant_signature = &shader_desc->patch_constant_signature;
         const struct shader_signature *output_signature = &shader_desc->output_signature;

    Should that be "uint64_t config_flags"?

  • added 2 commits

    • f410c52c - vkd3d-shader/spirv: Honor force_validation after emitting SPIR-V code.
    • 2c8c28ae - ci: Build vkd3d with SPIRV-Tools.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 2 commits

    • 7aceb92b - vkd3d-shader/spirv: Honor force_validation after emitting SPIR-V code.
    • 8d6cf03b - ci: Build vkd3d with SPIRV-Tools.

    Compare with previous version

  • Alexandre Julliard added 26 commits

    added 26 commits

    • 8d6cf03b...1015cc95 - 23 commits from branch wine:master
    • a6317e5f - vkd3d: Pad push constant ranges to 16 bytes.
    • ec4986e9 - vkd3d-shader/spirv: Honor force_validation after emitting SPIR-V code.
    • f087aa75 - ci: Build vkd3d with SPIRV-Tools.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading