[PATCH v2 0/1] MR170: vkd3d-shader/ir: Emit a shader error if the control point phase id cannot be inserted.

[Conor McCarthy (＠cmccarthy)]

A message context in the normaliser will also be needed for the next normalisation patch set.

--
  v2: vkd3d-shader/ir: Emit a shader error if the control point phase id cannot be inserted.

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

[PATCH v2 1/1] vkd3d-shader/ir: Emit a shader error if the control point phase id cannot be inserted.

[Conor McCarthy]

From: Conor McCarthy

---
 libs/vkd3d-shader/ir.c                   | 28 +++++++++++++++++++++---
 libs/vkd3d-shader/spirv.c                |  2 +-
 libs/vkd3d-shader/vkd3d_shader_private.h |  7 +++++-
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c
index 11ba7563..bf2c4442 100644
--- a/libs/vkd3d-shader/ir.c
+++ b/libs/vkd3d-shader/ir.c
@@ -18,6 +18,16 @@

#include "vkd3d_shader_private.h"

+static void VKD3D_PRINTF_FUNC(3, 4) shader_normaliser_error(struct vkd3d_shader_normaliser *normaliser,
+        enum vkd3d_shader_error error, const char *format, ...)
+{
+    va_list args;
+
+    va_start(args, format);
+    vkd3d_shader_verror(normaliser->message_context, &normaliser->location, error, format, args);
+    va_end(args);
+}
+
 static inline bool shader_register_is_phase_instance_id(const struct vkd3d_shader_register *reg)
 {
     return reg->type == VKD3DSPR_FORKINSTID || reg->type == VKD3DSPR_JOININSTID;
@@ -205,6 +215,7 @@ static enum vkd3d_result shader_normaliser_flatten_phases(struct vkd3d_shader_no
         {
             for (k = 0; k < loc->instruction_count; ++k)
             {
+                normaliser->location.line = loc->index + k + 1;
                 if (!shader_instruction_array_clone_instruction(&normaliser->instructions,
                         loc->index + loc->instruction_count * j + k, loc->index + k))
                     return VKD3D_ERROR_OUT_OF_MEMORY;
@@ -214,8 +225,11 @@ static enum vkd3d_result shader_normaliser_flatten_phases(struct vkd3d_shader_no
         for (j = 0; j < loc->instance_count; ++j)
         {
             for (k = 0; k < loc->instruction_count; ++k)
-                shader_instruction_eliminate_phase_instance_id(
-                        &normaliser->instructions.elements[loc->index + loc->instruction_count * j + k], j);
+            {
+                unsigned int ins_idx = loc->index + loc->instruction_count * j + k;
+                normaliser->location.line = ins_idx + 1;
+                shader_instruction_eliminate_phase_instance_id(&normaliser->instructions.elements[ins_idx], j);
+            }
         }
     }

@@ -245,11 +259,12 @@ static void shader_instruction_init(struct vkd3d_shader_instruction *ins, enum v
 }

void shader_normaliser_init(struct vkd3d_shader_normaliser *normaliser,
-        struct vkd3d_shader_instruction_array *instructions)
+        struct vkd3d_shader_instruction_array *instructions, struct vkd3d_shader_message_context *message_context)
 {
     memset(normaliser, 0, sizeof(*normaliser));
     normaliser->phase = VKD3DSIH_INVALID;
     normaliser->instructions = *instructions;
+    normaliser->message_context = message_context;
     memset(instructions, 0, sizeof(*instructions));
 }

@@ -261,7 +276,10 @@ enum vkd3d_result shader_normaliser_flatten_hull_shader_phases(struct vkd3d_shad
     unsigned int i;

for (i = 0, locations.count = 0; i < instructions->count; ++i)
+    {
+        normaliser->location.line = i + 1;
         shader_normaliser_eliminate_phase_related_dcls(normaliser, i, &locations);
+    }

if ((result = shader_normaliser_flatten_phases(normaliser, &locations)) < 0)
         return result;
@@ -303,6 +321,8 @@ static bool shader_dst_param_normalise_outpointid(struct vkd3d_shader_dst_param
         if (reg->idx[2].offset != ~0u)
         {
             FIXME("Cannot insert phase id.\n");
+            shader_normaliser_error(normaliser, VKD3D_SHADER_ERROR_IR_REGISTER_ORDER_TOO_HIGH,
+                    "Highest register order is occupied. Cannot insert phase id.\n");
             return false;
         }
         if (reg->idx[1].offset != ~0u)
@@ -397,6 +417,7 @@ enum vkd3d_result shader_normaliser_normalise_hull_shader_control_point_io(struc
     for (i = 0; i < normaliser->instructions.count; ++i)
     {
         ins = &instructions->elements[i];
+        normaliser->location.line = i + 1;

switch (ins->handler_idx)
         {
@@ -424,6 +445,7 @@ enum vkd3d_result shader_normaliser_normalise_hull_shader_control_point_io(struc
     for (i = 0; i < instructions->count; ++i)
     {
         ins = &instructions->elements[i];
+        normaliser->location.line = i + 1;

switch (ins->handler_idx)
         {
diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c
index 73510f8b..03a713e9 100644
--- a/libs/vkd3d-shader/spirv.c
+++ b/libs/vkd3d-shader/spirv.c
@@ -9696,7 +9696,7 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler,

if (compiler->shader_type == VKD3D_SHADER_TYPE_HULL)
     {
-        shader_normaliser_init(&normaliser, instructions);
+        shader_normaliser_init(&normaliser, instructions, compiler->message_context);
         if ((result = shader_normaliser_flatten_hull_shader_phases(&normaliser)) >= 0)
             result = shader_normaliser_normalise_hull_shader_control_point_io(&normaliser,
                     &parser->shader_desc.input_signature);
diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h
index e635a70b..88b79fea 100644
--- a/libs/vkd3d-shader/vkd3d_shader_private.h
+++ b/libs/vkd3d-shader/vkd3d_shader_private.h
@@ -139,6 +139,8 @@ enum vkd3d_shader_error
     VKD3D_SHADER_ERROR_D3DBC_OUT_OF_MEMORY              = 7004,

VKD3D_SHADER_WARNING_D3DBC_IGNORED_INSTRUCTION_FLAGS= 7300,
+
+    VKD3D_SHADER_ERROR_IR_REGISTER_ORDER_TOO_HIGH       = 8000,
 };

enum vkd3d_shader_opcode
@@ -1331,6 +1333,9 @@ int dxbc_writer_write(struct dxbc_writer *dxbc, struct vkd3d_shader_code *code);

struct vkd3d_shader_normaliser
 {
+    struct vkd3d_shader_message_context *message_context;
+    struct vkd3d_shader_location location;
+
     struct vkd3d_shader_instruction_array instructions;

unsigned int max_temp_count;
@@ -1344,7 +1349,7 @@ struct vkd3d_shader_normaliser
 };

void shader_normaliser_init(struct vkd3d_shader_normaliser *normaliser,
-        struct vkd3d_shader_instruction_array *instructions);
+        struct vkd3d_shader_instruction_array *instructions, struct vkd3d_shader_message_context *message_context);
 enum vkd3d_result shader_normaliser_flatten_hull_shader_phases(struct vkd3d_shader_normaliser *normaliser);
 enum vkd3d_result shader_normaliser_normalise_hull_shader_control_point_io(struct vkd3d_shader_normaliser *normaliser,
         const struct vkd3d_shader_signature *input_signature);

[Henri Verbeet (＠hverbeet)]

I don't think it really makes sense to track the location in the vkd3d_shader_normaliser structure. After a couple of transformations, the instruction index may no longer have much of a relation to the original source location. I think we should instead store location information in the vkd3d_shader_instruction structure, much like how the HLSL compiler stores location information in e.g. the hlsl_ir_node structure. Eventually, that should also make it straightforward to emit SpvOpLine instructions in the SPIR-V backend, giving us useful debug information in the generated SPIR-V.

I'm not quite sure about VKD3D_SHADER_ERROR_IR_REGISTER_ORDER_TOO_HIGH. On the one hand, it makes sense that we'd have our own errors on the IR level. On the other hand, this feels like something the TPF frontend should have caught and emitted an error for while parsing. Perhaps others have opinions?

[Zebediah Figura (＠zfigura)]

> I'm not quite sure about VKD3D_SHADER_ERROR_IR_REGISTER_ORDER_TOO_HIGH. On the one hand, it makes sense that we'd have our own errors on the IR level. On the other hand, this feels like something the TPF frontend should have caught and emitted an error for while parsing. Perhaps others have opinions?

That does sound like something the DXBC frontend should validate rather than the normalizer.

[PATCH v2 0/1] MR170: vkd3d-shader/ir: Emit a shader error if the control point phase id cannot be inserted. - closed

[Conor McCarthy (＠cmccarthy)]

This merge request was closed by Conor McCarthy.