[Mesa-dev] [PATCH 14/22] nir/linker: add some cross stage uniform validation

[Alejandro Piñeiro]

For now it checks that the type and array size of the uniforms is the
same, as right now only this and uniform->opaque is filled (and this
is specific for each stage). More checks will be added as more
features get added.

We are not checking for the same explicit location as we are basing
the linking on the location.

We are also avoiding name-checks. As they are just debug info, nothing
prevents to have one SPIR-V module with the names and other without
it. It also seems reasonable to think that it is not even needed to
keep the same names on different modules/stages.

As mentioned on the comment included with this patch, on GLSL linker
there are some full cross-stage validation methods, taking into
account more than just the uniforms. It is likely that this gets moved
later to a bigger method like that.
---
 src/compiler/nir/nir_link_uniforms.c | 68 +++++++++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 8 deletions(-)

diff --git a/src/compiler/nir/nir_link_uniforms.c b/src/compiler/nir/nir_link_uniforms.c
index af01a83a8e3..754385967f6 100644
--- a/src/compiler/nir/nir_link_uniforms.c
+++ b/src/compiler/nir/nir_link_uniforms.c
@@ -215,6 +215,21 @@ get_next_index(struct nir_link_uniforms_state *state,
    return index;
 }
 
+static void
+handle_type_for_uniform_storage(const struct glsl_type *original_type,
+                                const struct glsl_type **storage_type,
+                                unsigned *array_elements)
+{
+
+   const struct glsl_type *type_no_array = glsl_without_array(original_type);
+   if (glsl_type_is_array(original_type)) {
+      *storage_type = type_no_array;
+      *array_elements = glsl_get_length(original_type);
+   } else {
+      *storage_type = original_type;
+      *array_elements = 0;
+   }
+}
 
 /**
  * Creates the neccessary entries in UniformStorage for the uniform. Returns
@@ -292,14 +307,8 @@ nir_link_uniform(struct gl_context *ctx,
        */
       uniform->name = NULL;
 
-      const struct glsl_type *type_no_array = glsl_without_array(type);
-      if (glsl_type_is_array(type)) {
-         uniform->type = type_no_array;
-         uniform->array_elements = glsl_get_length(type);
-      } else {
-         uniform->type = type;
-         uniform->array_elements = 0;
-      }
+      handle_type_for_uniform_storage(type, &uniform->type,
+                                      &uniform->array_elements);
       uniform->active_shader_mask |= 1 << stage;
 
       /* Uniform has an explicit location */
@@ -327,6 +336,7 @@ nir_link_uniform(struct gl_context *ctx,
 
       unsigned entries = MAX2(1, uniform->array_elements);
 
+      const struct glsl_type *type_no_array = glsl_without_array(type);
       if (glsl_type_is_sampler(type_no_array)) {
          int sampler_index =
             get_next_index(state, uniform, &state->next_sampler_index);
@@ -383,6 +393,45 @@ nir_link_uniform(struct gl_context *ctx,
    }
 }
 
+/*
+ * Validate if the uniform already linked before on @existing is compatible
+ * with the new usage of it on another stage at @current.
+ *
+ * GLSL linker has specific methods for validation after the linkage, so
+ * probably this bit will be moved later to a more general one.
+ */
+static bool
+cross_validate_uniform(struct gl_shader_program *prog,
+                       struct gl_uniform_storage* existing,
+                       nir_variable *current)
+{
+   const struct glsl_type *type = current->type;
+   unsigned array_elements = 0;
+
+   handle_type_for_uniform_storage(current->type, &type, &array_elements);
+
+   if (existing->type != type) {
+      linker_error(prog, "uniform with location %i declared as "
+                   "type `%s' and type `%s'\n", current->data.location,
+                   glsl_get_type_name(type),
+                   glsl_get_type_name(existing->type));
+
+      return false;
+   }
+
+   if (existing->array_elements != array_elements) {
+      linker_error(prog, "uniform with location %i declared as "
+                   "type `%s[%i]' and type `%s[%i]'\n", current->data.location,
+                   glsl_get_type_name(type), array_elements,
+                   glsl_get_type_name(existing->type), existing->array_elements);
+
+      return false;
+   }
+
+   return true;
+}
+
+
 bool
 nir_link_uniforms(struct gl_context *ctx,
                   struct gl_shader_program *prog)
@@ -418,6 +467,9 @@ nir_link_uniforms(struct gl_context *ctx,
           */
          uniform = find_previous_uniform_storage(prog, var->data.location);
          if (uniform) {
+            if (!cross_validate_uniform(prog, uniform, var))
+               return false;
+
             uniform->active_shader_mask |= 1 << shader_type;
 
             continue;
-- 
2.14.1