[PATCH v2 0/2] MR345: vkd3d-shader/dxil: Fix a couple of issues.

[Conor McCarthy (＠cmccarthy)]

--
  v2: vkd3d-shader/dxil: Do not compile compute shaders.

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

[PATCH v2 1/2] vkd3d-shader/dxil: Do not access null code blocks on failure.

[Conor McCarthy]

From: Conor McCarthy

---
 libs/vkd3d-shader/dxil.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c
index 666d8b08..5f332f59 100644
--- a/libs/vkd3d-shader/dxil.c
+++ b/libs/vkd3d-shader/dxil.c
@@ -2418,11 +2418,11 @@ static enum vkd3d_result sm6_parser_function_init(struct sm6_parser *sm6, const
         struct sm6_function *function)
 {
     struct vkd3d_shader_instruction *ins;
+    size_t i, block_idx, block_count;
     const struct dxil_record *record;
     bool ret_found, is_terminator;
     struct sm6_block *code_block;
     struct sm6_value *dst;
-    size_t i, block_idx;

if (sm6->function_count)
     {
@@ -2448,12 +2448,12 @@ static enum vkd3d_result sm6_parser_function_init(struct sm6_parser *sm6, const
         return VKD3D_ERROR_INVALID_SHADER;
     }

-    if (!(function->block_count = block->records[0]->operands[0]))
+    if (!(block_count = block->records[0]->operands[0]))
     {
         WARN("Function contains no blocks.\n");
         return VKD3D_ERROR_INVALID_SHADER;
     }
-    if (function->block_count > 1)
+    if (block_count > 1)
     {
         FIXME("Branched shaders are not supported yet.\n");
         return VKD3D_ERROR_INVALID_SHADER;
@@ -2464,6 +2464,7 @@ static enum vkd3d_result sm6_parser_function_init(struct sm6_parser *sm6, const
         ERR("Failed to allocate code block.\n");
         return VKD3D_ERROR_OUT_OF_MEMORY;
     }
+    function->block_count = block_count;
     code_block = function->blocks[0];

sm6->cur_max_value = function->value_count;

[PATCH v2 2/2] vkd3d-shader/dxil: Do not compile compute shaders.

[Conor McCarthy]

From: Conor McCarthy

The thread group size is not yet emitted.
---
 libs/vkd3d-shader/dxil.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c
index 5f332f59..f0acde27 100644
--- a/libs/vkd3d-shader/dxil.c
+++ b/libs/vkd3d-shader/dxil.c
@@ -2760,6 +2760,11 @@ static enum vkd3d_result sm6_parser_init(struct sm6_parser *sm6, const uint32_t
         vkd3d_shader_parser_warning(&sm6->p, VKD3D_SHADER_WARNING_DXIL_UNKNOWN_SHADER_TYPE,
                 "Unknown shader type %#x.", version.type);
     }
+    if (version.type == VKD3D_SHADER_TYPE_COMPUTE)
+    {
+        FIXME("Compute shader compilation is not implemented yet.\n");
+        return VKD3D_ERROR_INVALID_SHADER;
+    }

version.major = VKD3D_SM6_VERSION_MAJOR(version_token);
     version.minor = VKD3D_SM6_VERSION_MINOR(version_token);

[Conor McCarthy (＠cmccarthy)]

On Thu Sep 21 04:44:02 2023 +0000, Henri Verbeet wrote:
> > The thread group size is not parsed at all, so this is just to prevent
>
> a crash. I'd rather not do it, but unless we avoid SM6 compute shaders
> in shader runner for now, I see no way around it.
> Why do those shaders end up compiling at all?

The group size is located in the metadata, loading of which will be included in the next MR. Until then, compute shaders can't reasonably be compiled.

[Conor McCarthy (＠cmccarthy)]

This should include some shader runner test modifications if MR346 is upstreamed first.

[Henri Verbeet (＠hverbeet)]

> The group size is located in the metadata, loading of which will be included in the next MR. Until then, compute shaders can't reasonably be compiled.

Right, but it looks like we compile them anyway without this patch, and just emit incorrect/invalid SPIR-V. I.e., I think sm6_parser_emit_unhandled() for example should be an error, and result in compilation failing. Similarly, if any metadata blocks are required to be parsed in order to produce correct output, not parsing them should be an error. Failing compilation is generally better than (in some cases silently) miscompiling code.

[Conor McCarthy (＠cmccarthy)]

On Thu Sep 21 13:30:49 2023 +0000, Henri Verbeet wrote:
> > The group size is located in the metadata, loading of which will be
>
> included in the next MR. Until then, compute shaders can't reasonably be compiled.
> Right, but it looks like we compile them anyway without this patch, and
> just emit incorrect/invalid SPIR-V. I.e., I think
> sm6_parser_emit_unhandled() for example should be an error, and result
> in compilation failing. Similarly, if any metadata blocks are required
> to be parsed in order to produce correct output, not parsing them should
> be an error. Failing compilation is generally better than (in some cases
> silently) miscompiling code.

The current situation with metadata is a little complicated. Strictly speaking the signatures should be loaded from metadata because the DXBC signatures may not be identical, and the interpolation mode is not loaded either. There's no issue with simple signatures though. I think the best thing to do is upstream metadata and signature loads ASAP.

`sm6_parser_emit_unhandled()` was useful during development, but yeah it should be an error now.