[PATCH v2 0/1] MR261: vkd3d-shader: Add compiler option to specify matrix majority.

[Nikolay Sivov (＠nsivov)]

Signed-off-by: Nikolay Sivov

--
  v2: vkd3d-shader: Add compiler option to specify matrix majority.

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

[PATCH v2 1/1] vkd3d-shader: Add compiler option to specify matrix majority.

[Nikolay Sivov]

From: Nikolay Sivov

Signed-off-by: Nikolay Sivov 
---
 include/vkd3d_shader.h   | 17 +++++++++++++++++
 libs/vkd3d-shader/hlsl.c | 21 ++++++++++++++++++---
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h
index 6c17a07b..77e6f53a 100644
--- a/include/vkd3d_shader.h
+++ b/include/vkd3d_shader.h
@@ -139,6 +139,14 @@ enum vkd3d_shader_compile_option_formatting_flags
     VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_COMPILE_OPTION_FORMATTING_FLAGS),
 };

+enum vkd3d_shader_compile_option_pack_matrix_order
+{
+    VKD3D_SHADER_COMPILE_OPTION_PACK_MATRIX_ROW_MAJOR    = 0x00000001,
+    VKD3D_SHADER_COMPILE_OPTION_PACK_MATRIX_COLUMN_MAJOR = 0x00000002,
+
+    VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_COMPILE_OPTION_PACK_MATRIX_ORDER),
+};
+
 enum vkd3d_shader_compile_option_name
 {
     /**
@@ -169,6 +177,15 @@ enum vkd3d_shader_compile_option_name
      * \since 1.7
      */
     VKD3D_SHADER_COMPILE_OPTION_WRITE_TESS_GEOM_POINT_SIZE = 0x00000006,
+    /**
+     * This option specifies default matrix packing order. It's only supported for HLSL source type.
+     * Explicit variable modifiers or pragmas will take precedence.
+     *
+     * \a value is a member of enum vkd3d_shader_compile_option_pack_matrix_order.
+     *
+     * \since 1.9
+     */
+    VKD3D_SHADER_COMPILE_OPTION_PACK_MATRIX_ORDER = 0x00000007,

VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_COMPILE_OPTION_NAME),
 };
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index 3a1e1779..27abd408 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -3208,9 +3208,11 @@ static void declare_predefined_types(struct hlsl_ctx *ctx)
     }
 }

-static bool hlsl_ctx_init(struct hlsl_ctx *ctx, const char *source_name,
+static bool hlsl_ctx_init(struct hlsl_ctx *ctx, const struct vkd3d_shader_compile_info *compile_info,
         const struct hlsl_profile_info *profile, struct vkd3d_shader_message_context *message_context)
 {
+    unsigned int i;
+
     memset(ctx, 0, sizeof(*ctx));

ctx->profile = profile;
@@ -3219,7 +3221,7 @@ static bool hlsl_ctx_init(struct hlsl_ctx *ctx, const char *source_name,

if (!(ctx->source_files = hlsl_alloc(ctx, sizeof(*ctx->source_files))))
         return false;
-    if (!(ctx->source_files[0] = hlsl_strdup(ctx, source_name ? source_name : "<anonymous>")))
+    if (!(ctx->source_files[0] = hlsl_strdup(ctx, compile_info->source_name ? compile_info->source_name : "<anonymous>")))
     {
         vkd3d_free(ctx->source_files);
         return false;
@@ -3258,6 +3260,19 @@ static bool hlsl_ctx_init(struct hlsl_ctx *ctx, const char *source_name,
         return false;
     ctx->cur_buffer = ctx->globals_buffer;

+    for (i = 0; i < compile_info->option_count; ++i)
+    {
+        const struct vkd3d_shader_compile_option *option = &compile_info->options[i];
+
+        if (option->name == VKD3D_SHADER_COMPILE_OPTION_PACK_MATRIX_ORDER)
+        {
+            if (option->value == VKD3D_SHADER_COMPILE_OPTION_PACK_MATRIX_ROW_MAJOR)
+                ctx->matrix_majority = HLSL_MODIFIER_ROW_MAJOR;
+            else if (option->value == VKD3D_SHADER_COMPILE_OPTION_PACK_MATRIX_COLUMN_MAJOR)
+                ctx->matrix_majority = HLSL_MODIFIER_COLUMN_MAJOR;
+        }
+    }
+
     return true;
 }

@@ -3333,7 +3348,7 @@ int hlsl_compile_shader(const struct vkd3d_shader_code *hlsl, const struct vkd3d
         return VKD3D_ERROR_INVALID_ARGUMENT;
     }

-    if (!hlsl_ctx_init(&ctx, compile_info->source_name, profile, message_context))
+    if (!hlsl_ctx_init(&ctx, compile_info, profile, message_context))
         return VKD3D_ERROR_OUT_OF_MEMORY;

if ((ret = hlsl_lexer_compile(&ctx, hlsl)) == 2)

[Nikolay Sivov (＠nsivov)]

On Tue Jul  4 14:18:05 2023 +0000, Henri Verbeet wrote:
> This is only supported/used for HLSL sources, and I think the
> documentation should reflect that. (See e.g. the documentation for
> VKD3D_SHADER_COMPILE_OPTION_WRITE_TESS_GEOM_POINT_SIZE.) The
> documentation for some of the existing compilation options probably has
> room for improvement as well.

Thanks, I pushed a comment for that.

[Giovanni Mascellani (＠giomasce)]

At first sight this doesn't look wrong, but previous tests (see `tests/hlsl/majority_*.shader_test`) show that the way majority interacts with `pragma`'s and `typedef`'s is quite complicated. In particular, there are some subtle differences between having column majority as a result of setting it explicitly via a `pragma` and as a result of not having set any other default. How that interacts with the majority set via a compilation flag is therefore not necessarily obvious (i.e., is explicitly setting a column majority via a compilation flag equivalent to setting it via `pragma`, or to leaving it to the unset default?), and for that reason it would be particularly appropriate to have tests for that.

[Nikolay Sivov (＠nsivov)]

The only reasonably fast way to test this would be with d3dcompiler directly.

[Zebediah Figura (＠zfigura)]

Yes, we need tests for this. In hlsl_d3d12.c would work, but we also can't be afraid to add tests to the shader-runner infrastructure just because there's no support there yet.

[Nikolay Sivov (＠nsivov)]

Any preferences on how to arrange that in .shader_test files? Following [require] block, it could be also global option, or something per [shader].

[Nikolay Sivov (＠nsivov)]

Options do work differently, they simply set default packing and produce error if different packing was specified in some other way.

[Zebediah Figura (＠zfigura)]

On Thu Jul  6 15:21:10 2023 +0000, Nikolay Sivov wrote:
> Any preferences on how to arrange that in .shader_test files? Following
> [require] block, it could be also global option, or something per [shader].

Not really. Putting it in [require] seems reasonable, although "require" is a bit of a misnomer.

[Nikolay Sivov (＠nsivov)]

I posted some tests as wine!3263, what I think that shows is compiling options acting exactly as corresponding pragmas, when placed in front of the source file. Following pragmas will override the options, conflicting modifiers will produce errors, same as pragmas should be doing already.

Another aspect is that you can't have both options on, but I think it's easier to filter that on wine side, keeping unnecessary validation out of the library.