[PATCH 0/6] MR679: vkd3d-shader: Refactor common code between vkd3d_shader_compile() and vkd3d_shader_scan().

List overview All Threads

newer

older

[PATCH v2 0/1] MR5167: quartz: Set...

Re: [PATCH v2 0/6] MR676:...

Giovanni Mascellani (＠giomasce)

1 Mar 2024 1 Mar '24

11:55 a.m.

-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679

Show replies by date

Giovanni Mascellani

1 Mar 1 Mar

11:55 a.m.

New subject: [PATCH 1/6] vkd3d-shader: Do not validate the source type twice.

From: Giovanni Mascellani gmascellani@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

Giovanni Mascellani

11:55 a.m.

New subject: [PATCH 2/6] vkd3d-shader: Treat the HLSL case specially in vkd3d_shader_scan().

From: Giovanni Mascellani gmascellani@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

Giovanni Mascellani

11:55 a.m.

New subject: [PATCH 3/6] vkd3d-shader: Treat the HLSL case specially in vkd3d_shader_compile().

From: Giovanni Mascellani gmascellani@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

Giovanni Mascellani

11:55 a.m.

New subject: [PATCH 4/6] vkd3d-shader: Refactor common code for vkd3d_shader_scan().

From: Giovanni Mascellani gmascellani@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

Giovanni Mascellani

11:55 a.m.

New subject: [PATCH 5/6] vkd3d-shader: Refactor common code for vkd3d_shader_compile().

From: Giovanni Mascellani gmascellani@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

Giovanni Mascellani

11:55 a.m.

New subject: [PATCH 6/6] vkd3d-shader: Refactor common code between vkd3d_shader_compile() and vkd3d_shader_scan().

From: Giovanni Mascellani gmascellani@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

Giovanni Mascellani (＠giomasce)

5 Mar 5 Mar

2:11 p.m.

New subject: [PATCH 0/6] MR679: vkd3d-shader: Refactor common code between vkd3d_shader_compile() and vkd3d_shader_scan(). - approved

This merge request was approved by Giovanni Mascellani.

-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/679

Henri Verbeet (＠hverbeet)

6 Mar 6 Mar

7:16 p.m.

...

-            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

Giovanni Mascellani (＠giomasce)

10:31 p.m.

New subject: [PATCH 0/5] MR679: vkd3d-shader: Refactor common code between vkd3d_shader_compile() and vkd3d_shader_scan().

...

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

339

Age (days ago)

344

Last active (days ago)

wine-gitlab@winehq.org

9 comments

3 participants

tags (0)

participants (3)

Giovanni Mascellani
Giovanni Mascellani (＠giomasce)
Henri Verbeet (＠hverbeet)