[PATCH 0/1] MR46: vkd3d-shader/spirv: Do not write the point size if shaderTessellationAndGeometryPointSize is disabled.

[PATCH 1/1] vkd3d-shader/spirv: Do not write the point size if shaderTessellationAndGeometryPointSize is disabled.

[Zebediah Figura]

From: Zebediah Figura

---
 include/vkd3d_shader.h    |  6 ++++++
 libs/vkd3d-shader/spirv.c | 28 +++++++++++++++++++++-------
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h
index 7178ac5f..63381d43 100644
--- a/include/vkd3d_shader.h
+++ b/include/vkd3d_shader.h
@@ -47,6 +47,7 @@ enum vkd3d_shader_api_version
     VKD3D_SHADER_API_VERSION_1_3,
     VKD3D_SHADER_API_VERSION_1_4,
     VKD3D_SHADER_API_VERSION_1_5,
+    VKD3D_SHADER_API_VERSION_1_6,

VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_API_VERSION),
 };
@@ -718,6 +719,11 @@ enum vkd3d_shader_spirv_extension
     VKD3D_SHADER_SPIRV_EXTENSION_EXT_DESCRIPTOR_INDEXING,
     /** \since 1.3 */
     VKD3D_SHADER_SPIRV_EXTENSION_EXT_STENCIL_EXPORT,
+    /**
+     * The shaderTessellationAndGeometryPointSize feature is enabled.
+     * \since 1.6
+     */
+    VKD3D_SHADER_SPIRV_FEATURE_TESSELLATION_AND_GEOMETRY_POINT_SIZE,

VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_SPIRV_EXTENSION),
 };
diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c
index 734ba315..66f3e7c0 100644
--- a/libs/vkd3d-shader/spirv.c
+++ b/libs/vkd3d-shader/spirv.c
@@ -2285,6 +2285,7 @@ struct spirv_compiler
     struct vkd3d_shader_spec_constant *spec_constants;
     size_t spec_constants_size;
     enum vkd3d_shader_compile_option_formatting_flags formatting;
+    enum vkd3d_shader_api_version api_version;

struct vkd3d_string_buffer_cache string_buffers;
 };
@@ -2351,6 +2352,7 @@ struct spirv_compiler *spirv_compiler_create(const struct vkd3d_shader_version *

compiler->formatting = VKD3D_SHADER_COMPILE_OPTION_FORMATTING_INDENT
             | VKD3D_SHADER_COMPILE_OPTION_FORMATTING_HEADER;
+    compiler->api_version = VKD3D_SHADER_API_VERSION_1_2;

for (i = 0; i < compile_info->option_count; ++i)
     {
@@ -2375,10 +2377,8 @@ struct spirv_compiler *spirv_compiler_create(const struct vkd3d_shader_version *
                 compiler->formatting = option->value;
                 break;

-            default:
-                WARN("Ignoring unrecognised option %#x with value %#x.\n", option->name, option->value);
-
             case VKD3D_SHADER_COMPILE_OPTION_API_VERSION:
+                compiler->api_version = option->value;
                 break;

case VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV:
@@ -2389,6 +2389,10 @@ struct spirv_compiler *spirv_compiler_create(const struct vkd3d_shader_version *
                 else
                     WARN("Ignoring unrecognised value %#x for option %#x.\n", option->value, option->name);
                 break;
+
+            default:
+                WARN("Ignoring unrecognised option %#x with value %#x.\n", option->name, option->value);
+                break;
         }
     }

@@ -6347,10 +6351,20 @@ static void spirv_compiler_emit_point_size(struct spirv_compiler *compiler)
     /* Set the point size. Point sprites are not supported in d3d10+, but
      * point primitives can still be used with e.g. stream output. Vulkan
      * requires the point size to always be explicitly defined when outputting
-     * points. */
-    vkd3d_spirv_build_op_store(&compiler->spirv_builder,
-            spirv_compiler_emit_builtin_variable(compiler, &point_size, SpvStorageClassOutput, 0),
-            spirv_compiler_get_constant_float(compiler, 1.0f), SpvMemoryAccessMaskNone);
+     * points.
+     *
+     * If shaderTessellationAndGeometryPointSize is disabled, we must not write
+     * PointSize for tessellation and geometry shaders. In that case the point
+     * size defaults to 1.0. */
+    if (compiler->api_version < VKD3D_SHADER_API_VERSION_1_6
+            || spirv_compiler_is_opengl_target(compiler) || compiler->shader_type == VKD3D_SHADER_TYPE_VERTEX
+            || spirv_compiler_is_target_extension_supported(compiler,
+                    VKD3D_SHADER_SPIRV_FEATURE_TESSELLATION_AND_GEOMETRY_POINT_SIZE))
+    {
+        vkd3d_spirv_build_op_store(&compiler->spirv_builder,
+                spirv_compiler_emit_builtin_variable(compiler, &point_size, SpvStorageClassOutput, 0),
+                spirv_compiler_get_constant_float(compiler, 1.0f), SpvMemoryAccessMaskNone);
+    }
 }

static void spirv_compiler_emit_dcl_output_topology(struct spirv_compiler *compiler,

[Henri Verbeet (＠hverbeet)]

I'm a bit undecided on VKD3D_SHADER_SPIRV_FEATURE_TESSELLATION_AND_GEOMETRY_POINT_SIZE. I don't hate it, but also can't help wondering whether it's the best we can do.

[Zebediah Figura (＠zfigura)]

On Fri Nov 18 00:04:04 2022 +0000, Henri Verbeet wrote:
> I'm a bit undecided on
> VKD3D_SHADER_SPIRV_FEATURE_TESSELLATION_AND_GEOMETRY_POINT_SIZE. I don't
> hate it, but also can't help wondering whether it's the best we can do.

It makes a lot of intuitive sense to me, in general, to treat features and extensions the same way. I don't think any better designs occur to me.

Of course, we already started down a different path with shaderStorageImageReadWithoutFormat in 3dbd2ceca. I still am not sure that was the best option, but perhaps we should just be consistent...

[Henri Verbeet (＠hverbeet)]

> It makes a lot of intuitive sense to me, in general, to treat features and extensions the same way. I don't think any better designs occur to me.
>
> Of course, we already started down a different path with shaderStorageImageReadWithoutFormat in 3dbd2ceca. I still am not sure that was the best option, but perhaps we should just be consistent...

That, but also whether it's really worth it to change the default value of this between releases. Those aren't entirely independent choices, of course; using the extension mechanism makes it a bit awkward to keep this enabled by default like it was in previous versions.