[PATCH 0/6] MR679: vkd3d-shader: Refactor common code between vkd3d_shader_compile() and vkd3d_shader_scan().
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/vkd3d_shader_main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 462a5c25e..086d11814 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1565,9 +1565,7 @@ int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char break; default: - ERR("Unsupported source type %#x.\n", compile_info->source_type); - ret = VKD3D_ERROR_INVALID_ARGUMENT; - break; + vkd3d_unreachable(); } vkd3d_shader_message_context_trace_messages(&message_context); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> The other cases are similar and common code can be refactored. Ideally the HLSL parser will eventually fit the same model, but that will require more extensive work. --- libs/vkd3d-shader/vkd3d_shader_main.c | 37 +++++++++++++++------------ 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 086d11814..9e08df4fc 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1545,27 +1545,30 @@ int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char vkd3d_shader_dump_shader(compile_info); - switch (compile_info->source_type) + if (compile_info->source_type == VKD3D_SHADER_SOURCE_HLSL) { - case VKD3D_SHADER_SOURCE_DXBC_TPF: - ret = scan_dxbc(compile_info, &message_context); - break; - - case VKD3D_SHADER_SOURCE_HLSL: - FIXME("HLSL support not implemented.\n"); - ret = VKD3D_ERROR_NOT_IMPLEMENTED; - break; + FIXME("HLSL support not implemented.\n"); + ret = VKD3D_ERROR_NOT_IMPLEMENTED; + } + else + { + switch (compile_info->source_type) + { + case VKD3D_SHADER_SOURCE_DXBC_TPF: + ret = scan_dxbc(compile_info, &message_context); + break; - case VKD3D_SHADER_SOURCE_D3D_BYTECODE: - ret = scan_d3dbc(compile_info, &message_context); - break; + case VKD3D_SHADER_SOURCE_D3D_BYTECODE: + ret = scan_d3dbc(compile_info, &message_context); + break; - case VKD3D_SHADER_SOURCE_DXBC_DXIL: - ret = scan_dxil(compile_info, &message_context); - break; + case VKD3D_SHADER_SOURCE_DXBC_DXIL: + ret = scan_dxil(compile_info, &message_context); + break; - default: - vkd3d_unreachable(); + default: + vkd3d_unreachable(); + } } vkd3d_shader_message_context_trace_messages(&message_context); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/vkd3d_shader_main.c | 35 +++++++++++++++------------ 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 9e08df4fc..5be6dc80c 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1716,26 +1716,29 @@ int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, vkd3d_shader_dump_shader(compile_info); - switch (compile_info->source_type) + if (compile_info->source_type == VKD3D_SHADER_SOURCE_HLSL) { - case VKD3D_SHADER_SOURCE_DXBC_TPF: - ret = compile_dxbc_tpf(compile_info, out, &message_context); - break; - - case VKD3D_SHADER_SOURCE_HLSL: - ret = compile_hlsl(compile_info, out, &message_context); - break; + ret = compile_hlsl(compile_info, out, &message_context); + } + else + { + switch (compile_info->source_type) + { + case VKD3D_SHADER_SOURCE_DXBC_TPF: + ret = compile_dxbc_tpf(compile_info, out, &message_context); + break; - case VKD3D_SHADER_SOURCE_D3D_BYTECODE: - ret = compile_d3d_bytecode(compile_info, out, &message_context); - break; + case VKD3D_SHADER_SOURCE_D3D_BYTECODE: + ret = compile_d3d_bytecode(compile_info, out, &message_context); + break; - case VKD3D_SHADER_SOURCE_DXBC_DXIL: - ret = compile_dxbc_dxil(compile_info, out, &message_context); - break; + case VKD3D_SHADER_SOURCE_DXBC_DXIL: + ret = compile_dxbc_dxil(compile_info, out, &message_context); + break; - default: - vkd3d_unreachable(); + default: + vkd3d_unreachable(); + } } vkd3d_shader_message_context_trace_messages(&message_context); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/vkd3d_shader_main.c | 76 ++++++--------------------- 1 file changed, 17 insertions(+), 59 deletions(-) diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 5be6dc80c..9a953252e 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1472,60 +1472,6 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info return ret; } -static int scan_dxbc(const struct vkd3d_shader_compile_info *compile_info, - struct vkd3d_shader_message_context *message_context) -{ - struct vkd3d_shader_parser *parser; - int ret; - - if ((ret = vkd3d_shader_sm4_parser_create(compile_info, message_context, &parser)) < 0) - { - WARN("Failed to initialise shader parser.\n"); - return ret; - } - - ret = scan_with_parser(compile_info, message_context, NULL, parser); - vkd3d_shader_parser_destroy(parser); - - return ret; -} - -static int scan_d3dbc(const struct vkd3d_shader_compile_info *compile_info, - struct vkd3d_shader_message_context *message_context) -{ - struct vkd3d_shader_parser *parser; - int ret; - - if ((ret = vkd3d_shader_sm1_parser_create(compile_info, message_context, &parser)) < 0) - { - WARN("Failed to initialise shader parser.\n"); - return ret; - } - - ret = scan_with_parser(compile_info, message_context, NULL, parser); - vkd3d_shader_parser_destroy(parser); - - return ret; -} - -static int scan_dxil(const struct vkd3d_shader_compile_info *compile_info, - struct vkd3d_shader_message_context *message_context) -{ - struct vkd3d_shader_parser *parser; - int ret; - - if ((ret = vkd3d_shader_sm6_parser_create(compile_info, message_context, &parser)) < 0) - { - WARN("Failed to initialise shader parser.\n"); - return ret; - } - - ret = scan_with_parser(compile_info, message_context, NULL, parser); - vkd3d_shader_parser_destroy(parser); - - return ret; -} - int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char **messages) { struct vkd3d_shader_message_context message_context; @@ -1552,23 +1498,35 @@ int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char } else { + struct vkd3d_shader_parser *parser; + switch (compile_info->source_type) { - case VKD3D_SHADER_SOURCE_DXBC_TPF: - ret = scan_dxbc(compile_info, &message_context); + case VKD3D_SHADER_SOURCE_D3D_BYTECODE: + ret = vkd3d_shader_sm1_parser_create(compile_info, &message_context, &parser); break; - case VKD3D_SHADER_SOURCE_D3D_BYTECODE: - ret = scan_d3dbc(compile_info, &message_context); + case VKD3D_SHADER_SOURCE_DXBC_TPF: + ret = vkd3d_shader_sm4_parser_create(compile_info, &message_context, &parser); break; case VKD3D_SHADER_SOURCE_DXBC_DXIL: - ret = scan_dxil(compile_info, &message_context); + ret = vkd3d_shader_sm6_parser_create(compile_info, &message_context, &parser); break; default: vkd3d_unreachable(); } + + if (ret < 0) + { + WARN("Failed to create shader parser.\n"); + } + else + { + ret = scan_with_parser(compile_info, &message_context, NULL, parser); + vkd3d_shader_parser_destroy(parser); + } } vkd3d_shader_message_context_trace_messages(&message_context); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/vkd3d_shader_main.c | 76 ++++++--------------------- 1 file changed, 17 insertions(+), 59 deletions(-) diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 9a953252e..3dcc99e2a 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1585,24 +1585,6 @@ static int vkd3d_shader_parser_compile(struct vkd3d_shader_parser *parser, return ret; } -static int compile_dxbc_tpf(const struct vkd3d_shader_compile_info *compile_info, - struct vkd3d_shader_code *out, struct vkd3d_shader_message_context *message_context) -{ - struct vkd3d_shader_parser *parser; - int ret; - - if ((ret = vkd3d_shader_sm4_parser_create(compile_info, message_context, &parser)) < 0) - { - WARN("Failed to initialise shader parser.\n"); - return ret; - } - - ret = vkd3d_shader_parser_compile(parser, compile_info, out, message_context); - - vkd3d_shader_parser_destroy(parser); - return ret; -} - static int compile_hlsl(const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_code *out, struct vkd3d_shader_message_context *message_context) { @@ -1618,42 +1600,6 @@ static int compile_hlsl(const struct vkd3d_shader_compile_info *compile_info, return ret; } -static int compile_d3d_bytecode(const struct vkd3d_shader_compile_info *compile_info, - struct vkd3d_shader_code *out, struct vkd3d_shader_message_context *message_context) -{ - struct vkd3d_shader_parser *parser; - int ret; - - if ((ret = vkd3d_shader_sm1_parser_create(compile_info, message_context, &parser)) < 0) - { - WARN("Failed to initialise shader parser.\n"); - return ret; - } - - ret = vkd3d_shader_parser_compile(parser, compile_info, out, message_context); - - vkd3d_shader_parser_destroy(parser); - return ret; -} - -static int compile_dxbc_dxil(const struct vkd3d_shader_compile_info *compile_info, - struct vkd3d_shader_code *out, struct vkd3d_shader_message_context *message_context) -{ - struct vkd3d_shader_parser *parser; - int ret; - - if ((ret = vkd3d_shader_sm6_parser_create(compile_info, message_context, &parser)) < 0) - { - WARN("Failed to initialise shader parser.\n"); - return ret; - } - - ret = vkd3d_shader_parser_compile(parser, compile_info, out, message_context); - - vkd3d_shader_parser_destroy(parser); - return ret; -} - int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_code *out, char **messages) { @@ -1680,23 +1626,35 @@ int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, } else { + struct vkd3d_shader_parser *parser; + switch (compile_info->source_type) { - case VKD3D_SHADER_SOURCE_DXBC_TPF: - ret = compile_dxbc_tpf(compile_info, out, &message_context); + case VKD3D_SHADER_SOURCE_D3D_BYTECODE: + ret = vkd3d_shader_sm1_parser_create(compile_info, &message_context, &parser); break; - case VKD3D_SHADER_SOURCE_D3D_BYTECODE: - ret = compile_d3d_bytecode(compile_info, out, &message_context); + case VKD3D_SHADER_SOURCE_DXBC_TPF: + ret = vkd3d_shader_sm4_parser_create(compile_info, &message_context, &parser); break; case VKD3D_SHADER_SOURCE_DXBC_DXIL: - ret = compile_dxbc_dxil(compile_info, out, &message_context); + ret = vkd3d_shader_sm6_parser_create(compile_info, &message_context, &parser); break; default: vkd3d_unreachable(); } + + if (ret < 0) + { + WARN("Failed to create shader parser.\n"); + } + else + { + ret = vkd3d_shader_parser_compile(parser, compile_info, out, &message_context); + vkd3d_shader_parser_destroy(parser); + } } vkd3d_shader_message_context_trace_messages(&message_context); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/vkd3d_shader_main.c | 102 ++++++++------------------ 1 file changed, 31 insertions(+), 71 deletions(-) diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 3dcc99e2a..1b60ac789 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1472,70 +1472,6 @@ static int scan_with_parser(const struct vkd3d_shader_compile_info *compile_info return ret; } -int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char **messages) -{ - struct vkd3d_shader_message_context message_context; - int ret; - - TRACE("compile_info %p, messages %p.\n", compile_info, messages); - - if (messages) - *messages = NULL; - - if ((ret = vkd3d_shader_validate_compile_info(compile_info, false)) < 0) - return ret; - - init_scan_signature_info(compile_info); - - vkd3d_shader_message_context_init(&message_context, compile_info->log_level); - - vkd3d_shader_dump_shader(compile_info); - - if (compile_info->source_type == VKD3D_SHADER_SOURCE_HLSL) - { - FIXME("HLSL support not implemented.\n"); - ret = VKD3D_ERROR_NOT_IMPLEMENTED; - } - else - { - struct vkd3d_shader_parser *parser; - - switch (compile_info->source_type) - { - case VKD3D_SHADER_SOURCE_D3D_BYTECODE: - ret = vkd3d_shader_sm1_parser_create(compile_info, &message_context, &parser); - break; - - case VKD3D_SHADER_SOURCE_DXBC_TPF: - ret = vkd3d_shader_sm4_parser_create(compile_info, &message_context, &parser); - break; - - case VKD3D_SHADER_SOURCE_DXBC_DXIL: - ret = vkd3d_shader_sm6_parser_create(compile_info, &message_context, &parser); - break; - - default: - vkd3d_unreachable(); - } - - if (ret < 0) - { - WARN("Failed to create shader parser.\n"); - } - else - { - ret = scan_with_parser(compile_info, &message_context, NULL, parser); - vkd3d_shader_parser_destroy(parser); - } - } - - vkd3d_shader_message_context_trace_messages(&message_context); - if (!vkd3d_shader_message_context_copy_messages(&message_context, messages)) - ret = VKD3D_ERROR_OUT_OF_MEMORY; - vkd3d_shader_message_context_cleanup(&message_context); - return ret; -} - static int vkd3d_shader_parser_compile(struct vkd3d_shader_parser *parser, const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_code *out, struct vkd3d_shader_message_context *message_context) @@ -1600,18 +1536,16 @@ static int compile_hlsl(const struct vkd3d_shader_compile_info *compile_info, return ret; } -int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, - struct vkd3d_shader_code *out, char **messages) +static int scan_or_compile(const struct vkd3d_shader_compile_info *compile_info, + struct vkd3d_shader_code *out, char **messages, bool compile) { struct vkd3d_shader_message_context message_context; int ret; - TRACE("compile_info %p, out %p, messages %p.\n", compile_info, out, messages); - if (messages) *messages = NULL; - if ((ret = vkd3d_shader_validate_compile_info(compile_info, true)) < 0) + if ((ret = vkd3d_shader_validate_compile_info(compile_info, compile)) < 0) return ret; init_scan_signature_info(compile_info); @@ -1622,7 +1556,15 @@ int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, if (compile_info->source_type == VKD3D_SHADER_SOURCE_HLSL) { - ret = compile_hlsl(compile_info, out, &message_context); + if (compile) + { + ret = compile_hlsl(compile_info, out, &message_context); + } + else + { + FIXME("HLSL support not implemented.\n"); + ret = VKD3D_ERROR_NOT_IMPLEMENTED; + } } else { @@ -1652,7 +1594,10 @@ int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, } else { - ret = vkd3d_shader_parser_compile(parser, compile_info, out, &message_context); + if (compile) + ret = vkd3d_shader_parser_compile(parser, compile_info, out, &message_context); + else + ret = scan_with_parser(compile_info, &message_context, NULL, parser); vkd3d_shader_parser_destroy(parser); } } @@ -1664,6 +1609,21 @@ int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, return ret; } +int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char **messages) +{ + TRACE("compile_info %p, messages %p.\n", compile_info, messages); + + return scan_or_compile(compile_info, NULL, messages, false); +} + +int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, + struct vkd3d_shader_code *out, char **messages) +{ + TRACE("compile_info %p, out %p, messages %p.\n", compile_info, out, messages); + + return scan_or_compile(compile_info, out, messages, true); +} + void vkd3d_shader_free_scan_combined_resource_sampler_info( struct vkd3d_shader_scan_combined_resource_sampler_info *info) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679
```diff - ERR("Unsupported source type %#x.\n", compile_info->source_type); - ret = VKD3D_ERROR_INVALID_ARGUMENT; - break; + vkd3d_unreachable(); ```
I imagine this is to make vkd3d_shader_scan() more similar to vkd3d_shader_compile(), but I think vkd3d_unreachable() is worse than the existing code. I feel a bit ambivalent about this MR. It's nice to get rid of some duplication and room for potential inconsistencies between vkd3d_shader_scan() and vkd3d_shader_compile(), but I don't love scan_or_compile(), and its "compile" argument in particular. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679#note_63735
I imagine this is to make vkd3d_shader_scan() more similar to vkd3d_shader_compile(), but I think vkd3d_unreachable() is worse than the existing code.
Even if the source type is already validated by `vkd3d_shader_validate_compile_info()`? Which use do you see into validating the source type twice?
I feel a bit ambivalent about this MR. It's nice to get rid of some duplication and room for potential inconsistencies between vkd3d_shader_scan() and vkd3d_shader_compile(), but I don't love scan_or_compile(), and its "compile" argument in particular.
I can see how that commit can be a bit more controversial than the others. Let's drop it for the moment. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679#note_63783
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)