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

[Giovanni Mascellani (＠giomasce)]

--
  v4: vkd3d-shader: Refactor common code for vkd3d_shader_compile().
      vkd3d-shader: Refactor common code for vkd3d_shader_scan().
      vkd3d-shader: Treat the HLSL case specially in vkd3d_shader_compile().
      vkd3d-shader: Treat the HLSL case specially in vkd3d_shader_scan().

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

[PATCH v4 1/4] vkd3d-shader: Treat the HLSL case specially in vkd3d_shader_scan().

[Giovanni Mascellani]

From: Giovanni Mascellani

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 | 41 ++++++++++++++-------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c
index 462a5c25e..0f36984b1 100644
--- a/libs/vkd3d-shader/vkd3d_shader_main.c
+++ b/libs/vkd3d-shader/vkd3d_shader_main.c
@@ -1545,29 +1545,32 @@ 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:
-            ERR("Unsupported source type %#x.\n", compile_info->source_type);
-            ret = VKD3D_ERROR_INVALID_ARGUMENT;
-            break;
+            default:
+                ERR("Unsupported source type %#x.\n", compile_info->source_type);
+                ret = VKD3D_ERROR_INVALID_ARGUMENT;
+                break;
+        }
     }

vkd3d_shader_message_context_trace_messages(&message_context);

[PATCH v4 2/4] vkd3d-shader: Treat the HLSL case specially in vkd3d_shader_compile().

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 libs/vkd3d-shader/vkd3d_shader_main.c | 37 +++++++++++++++------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c
index 0f36984b1..5d53bb2d1 100644
--- a/libs/vkd3d-shader/vkd3d_shader_main.c
+++ b/libs/vkd3d-shader/vkd3d_shader_main.c
@@ -1718,26 +1718,31 @@ 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:
+                ERR("Unsupported source type %#x.\n", compile_info->source_type);
+                ret = VKD3D_ERROR_INVALID_ARGUMENT;
+                break;
+        }
     }

vkd3d_shader_message_context_trace_messages(&message_context);

[PATCH v4 3/4] vkd3d-shader: Refactor common code for vkd3d_shader_scan().

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 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 5d53bb2d1..dd76bc06b 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,18 +1498,20 @@ 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:
@@ -1571,6 +1519,16 @@ int vkd3d_shader_scan(const struct vkd3d_shader_compile_info *compile_info, char
                 ret = VKD3D_ERROR_INVALID_ARGUMENT;
                 break;
         }
+
+        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);

[PATCH v4 4/4] vkd3d-shader: Refactor common code for vkd3d_shader_compile().

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 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 dd76bc06b..98e0168e5 100644
--- a/libs/vkd3d-shader/vkd3d_shader_main.c
+++ b/libs/vkd3d-shader/vkd3d_shader_main.c
@@ -1587,24 +1587,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)
 {
@@ -1620,42 +1602,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)
 {
@@ -1682,18 +1628,20 @@ 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:
@@ -1701,6 +1649,16 @@ int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info,
                 ret = VKD3D_ERROR_INVALID_ARGUMENT;
                 break;
         }
+
+        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);

[Giovanni Mascellani (＠giomasce)]

On Thu Mar  7 22:44:31 2024 +0000, Henri Verbeet wrote:
> > > 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?
> It's more about disliking vkd3d_unreachable()/abort(). That's a
> discussion we've had a few times before, of course; what's different
> this time is that we've seen a number of bugs come in through WineHQ
> Bugzilla with assertions failing inside vkd3d-shader since the last time
> we had that discussion.

Ok. I don't like it, but here it is.

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

[Henri Verbeet (＠hverbeet)]

This merge request was approved by Henri Verbeet.