Re: [Mesa-dev] [PATCH v2 03/11] mesa: Strip arrayness from interface block names in some IO validation

[Kenneth Graunke]

On Thursday, June 16, 2016 12:06:56 PM PDT Ian Romanick wrote:
> From: Ian Romanick 
> 
> Outputs from the vertex shader need to be able to match
> per-vertex-arrayed inputs of later stages.  Acomplish this by stripping
> one level of arrayness from the names and types of outputs going to a
> per-vertex-arrayed stage.
> 
> v2: Add missing checks for TESS_EVAL->GEOMETRY.  Noticed by Timothy
> Arceri.
> 
> Signed-off-by: Ian Romanick 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96358
> Cc: "12.0" 
> Cc: Gregory Hainaut 
> Cc: Ilia Mirkin 

This is an annoying amount of code, but it looks OK to me and I don't
have any ideas of how to do it better.

Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 03/11] mesa: Strip arrayness from interface block names in some IO validation

[Ian Romanick]

ping

I'll have a response to Ilia's feedback shortly.

On 06/16/2016 12:06 PM, Ian Romanick wrote:
> From: Ian Romanick 
> 
> Outputs from the vertex shader need to be able to match
> per-vertex-arrayed inputs of later stages.  Acomplish this by stripping
> one level of arrayness from the names and types of outputs going to a
> per-vertex-arrayed stage.
> 
> v2: Add missing checks for TESS_EVAL->GEOMETRY.  Noticed by Timothy
> Arceri.
> 
> Signed-off-by: Ian Romanick 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96358
> Cc: "12.0" 
> Cc: Gregory Hainaut 
> Cc: Ilia Mirkin 
> ---
>  src/mesa/main/shader_query.cpp | 100 
> +
>  1 file changed, 92 insertions(+), 8 deletions(-)
> 
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index 5956ce4..a6a0a2f 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -1385,13 +1385,26 @@ _mesa_get_program_resourceiv(struct gl_shader_program 
> *shProg,
>  
>  static bool
>  validate_io(struct gl_shader_program *producer,
> -struct gl_shader_program *consumer)
> +struct gl_shader_program *consumer,
> +gl_shader_stage producer_stage,
> +gl_shader_stage consumer_stage)
>  {
> if (producer == consumer)
>return true;
>  
> +   const bool nonarray_stage_to_array_stage =
> +  (producer_stage == MESA_SHADER_VERTEX &&
> +   (consumer_stage == MESA_SHADER_GEOMETRY ||
> +consumer_stage == MESA_SHADER_TESS_CTRL ||
> +consumer_stage == MESA_SHADER_TESS_EVAL)) ||
> +  (producer_stage == MESA_SHADER_TESS_EVAL &&
> +   consumer_stage == MESA_SHADER_GEOMETRY);
> +
> bool valid = true;
>  
> +   void *name_buffer = NULL;
> +   size_t name_buffer_size = 0;
> +
> gl_shader_variable const **outputs =
>(gl_shader_variable const **) calloc(producer->NumProgramResourceList,
> sizeof(gl_shader_variable *));
> @@ -1463,11 +1476,52 @@ validate_io(struct gl_shader_program *producer,
>  }
>   }
>} else {
> + char *consumer_name = consumer_var->name;
> +
> + if (nonarray_stage_to_array_stage &&
> + consumer_var->interface_type != NULL &&
> + consumer_var->interface_type->is_array() &&
> + !is_gl_identifier(consumer_var->name)) {
> +const size_t name_len = strlen(consumer_var->name);
> +
> +if (name_len >= name_buffer_size) {
> +   free(name_buffer);
> +
> +   name_buffer_size = name_len + 1;
> +   name_buffer = malloc(name_buffer_size);
> +   if (name_buffer == NULL) {
> +  valid = false;
> +  goto out;
> +   }
> +}
> +
> +consumer_name = (char *) name_buffer;
> +
> +char *s = strchr(consumer_var->name, '[');
> +if (s == NULL) {
> +   valid = false;
> +   goto out;
> +}
> +
> +char *t = strchr(s, ']');
> +if (t == NULL) {
> +   valid = false;
> +   goto out;
> +}
> +
> +assert(t[1] == '.' || t[1] == '[');
> +
> +const ptrdiff_t base_name_len = s - consumer_var->name;
> +
> +memcpy(consumer_name, consumer_var->name, base_name_len);
> +strcpy(consumer_name + base_name_len, t + 1);
> + }
> +
>   for (unsigned j = 0; j < num_outputs; j++) {
>  const gl_shader_variable *const var = outputs[j];
>  
>  if (!var->explicit_location &&
> -strcmp(consumer_var->name, var->name) == 0) {
> +strcmp(consumer_name, var->name) == 0) {
> producer_var = var;
> match_index = j;
> break;
> @@ -1529,25 +1583,53 @@ validate_io(struct gl_shader_program *producer,
> * Note that location mismatches are detected by the loops above that
> * find the producer variable that goes with the consumer variable.
> */
> -  if (producer_var->type != consumer_var->type ||
> -  producer_var->interpolation != consumer_var->interpolation ||
> -  producer_var->precision != consumer_var->precision) {
> +  if (nonarray_stage_to_array_stage) {
> + if (!consumer_var->type->is_array() ||
> + consumer_var->type->fields.array != producer_var->type) {
> +valid = false;
> +goto out;
> + }
> +
> + if (consumer_var->interface_type != NULL) {
> +if (!consumer_var->interface_type->is_array() ||
> +consumer_var->interface_type->fields.array != 
> producer_var->interface_type) {
> +   valid = false;
> + 

Re: [Mesa-dev] [PATCH v2 03/11] mesa: Strip arrayness from interface block names in some IO validation

[Ilia Mirkin]

On Thu, Jun 16, 2016 at 3:06 PM, Ian Romanick  wrote:
> From: Ian Romanick 
>
> Outputs from the vertex shader need to be able to match
> per-vertex-arrayed inputs of later stages.  Acomplish this by stripping
> one level of arrayness from the names and types of outputs going to a
> per-vertex-arrayed stage.
>
> v2: Add missing checks for TESS_EVAL->GEOMETRY.  Noticed by Timothy
> Arceri.
>
> Signed-off-by: Ian Romanick 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96358
> Cc: "12.0" 
> Cc: Gregory Hainaut 
> Cc: Ilia Mirkin 
> ---
>  src/mesa/main/shader_query.cpp | 100 
> +
>  1 file changed, 92 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index 5956ce4..a6a0a2f 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -1385,13 +1385,26 @@ _mesa_get_program_resourceiv(struct gl_shader_program 
> *shProg,
>
>  static bool
>  validate_io(struct gl_shader_program *producer,
> -struct gl_shader_program *consumer)
> +struct gl_shader_program *consumer,
> +gl_shader_stage producer_stage,
> +gl_shader_stage consumer_stage)
>  {
> if (producer == consumer)
>return true;
>
> +   const bool nonarray_stage_to_array_stage =
> +  (producer_stage == MESA_SHADER_VERTEX &&
> +   (consumer_stage == MESA_SHADER_GEOMETRY ||
> +consumer_stage == MESA_SHADER_TESS_CTRL ||
> +consumer_stage == MESA_SHADER_TESS_EVAL)) ||
> +  (producer_stage == MESA_SHADER_TESS_EVAL &&
> +   consumer_stage == MESA_SHADER_GEOMETRY);

Since TCS -> GEOM is not possible, isn't the only way that array ->
array can happen is TCS -> TES? IOW, couldn't this just be

producer != TCS && (consumer == TCS || consumer == TES || consumer == GS)

Your call whether you want to rewrite it. [No comment on the rest of
the patch...]

  -ilia

> +
> bool valid = true;
>
> +   void *name_buffer = NULL;
> +   size_t name_buffer_size = 0;
> +
> gl_shader_variable const **outputs =
>(gl_shader_variable const **) calloc(producer->NumProgramResourceList,
> sizeof(gl_shader_variable *));
> @@ -1463,11 +1476,52 @@ validate_io(struct gl_shader_program *producer,
>  }
>   }
>} else {
> + char *consumer_name = consumer_var->name;
> +
> + if (nonarray_stage_to_array_stage &&
> + consumer_var->interface_type != NULL &&
> + consumer_var->interface_type->is_array() &&
> + !is_gl_identifier(consumer_var->name)) {
> +const size_t name_len = strlen(consumer_var->name);
> +
> +if (name_len >= name_buffer_size) {
> +   free(name_buffer);
> +
> +   name_buffer_size = name_len + 1;
> +   name_buffer = malloc(name_buffer_size);
> +   if (name_buffer == NULL) {
> +  valid = false;
> +  goto out;
> +   }
> +}
> +
> +consumer_name = (char *) name_buffer;
> +
> +char *s = strchr(consumer_var->name, '[');
> +if (s == NULL) {
> +   valid = false;
> +   goto out;
> +}
> +
> +char *t = strchr(s, ']');
> +if (t == NULL) {
> +   valid = false;
> +   goto out;
> +}
> +
> +assert(t[1] == '.' || t[1] == '[');
> +
> +const ptrdiff_t base_name_len = s - consumer_var->name;
> +
> +memcpy(consumer_name, consumer_var->name, base_name_len);
> +strcpy(consumer_name + base_name_len, t + 1);
> + }
> +
>   for (unsigned j = 0; j < num_outputs; j++) {
>  const gl_shader_variable *const var = outputs[j];
>
>  if (!var->explicit_location &&
> -strcmp(consumer_var->name, var->name) == 0) {
> +strcmp(consumer_name, var->name) == 0) {
> producer_var = var;
> match_index = j;
> break;
> @@ -1529,25 +1583,53 @@ validate_io(struct gl_shader_program *producer,
> * Note that location mismatches are detected by the loops above that
> * find the producer variable that goes with the consumer variable.
> */
> -  if (producer_var->type != consumer_var->type ||
> -  producer_var->interpolation != consumer_var->interpolation ||
> -  producer_var->precision != consumer_var->precision) {
> +  if (nonarray_stage_to_array_stage) {
> + if (!consumer_var->type->is_array() ||
> + consumer_var->type->fields.array != producer_var->type) {
> +valid = false;
> +goto out;
> + }
> +
> + 

[Mesa-dev] [PATCH v2 03/11] mesa: Strip arrayness from interface block names in some IO validation

[Ian Romanick]

From: Ian Romanick 

Outputs from the vertex shader need to be able to match
per-vertex-arrayed inputs of later stages.  Acomplish this by stripping
one level of arrayness from the names and types of outputs going to a
per-vertex-arrayed stage.

v2: Add missing checks for TESS_EVAL->GEOMETRY.  Noticed by Timothy
Arceri.

Signed-off-by: Ian Romanick 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96358
Cc: "12.0" 
Cc: Gregory Hainaut 
Cc: Ilia Mirkin 
---
 src/mesa/main/shader_query.cpp | 100 +
 1 file changed, 92 insertions(+), 8 deletions(-)

diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
index 5956ce4..a6a0a2f 100644
--- a/src/mesa/main/shader_query.cpp
+++ b/src/mesa/main/shader_query.cpp
@@ -1385,13 +1385,26 @@ _mesa_get_program_resourceiv(struct gl_shader_program 
*shProg,
 
 static bool
 validate_io(struct gl_shader_program *producer,
-struct gl_shader_program *consumer)
+struct gl_shader_program *consumer,
+gl_shader_stage producer_stage,
+gl_shader_stage consumer_stage)
 {
if (producer == consumer)
   return true;
 
+   const bool nonarray_stage_to_array_stage =
+  (producer_stage == MESA_SHADER_VERTEX &&
+   (consumer_stage == MESA_SHADER_GEOMETRY ||
+consumer_stage == MESA_SHADER_TESS_CTRL ||
+consumer_stage == MESA_SHADER_TESS_EVAL)) ||
+  (producer_stage == MESA_SHADER_TESS_EVAL &&
+   consumer_stage == MESA_SHADER_GEOMETRY);
+
bool valid = true;
 
+   void *name_buffer = NULL;
+   size_t name_buffer_size = 0;
+
gl_shader_variable const **outputs =
   (gl_shader_variable const **) calloc(producer->NumProgramResourceList,
sizeof(gl_shader_variable *));
@@ -1463,11 +1476,52 @@ validate_io(struct gl_shader_program *producer,
 }
  }
   } else {
+ char *consumer_name = consumer_var->name;
+
+ if (nonarray_stage_to_array_stage &&
+ consumer_var->interface_type != NULL &&
+ consumer_var->interface_type->is_array() &&
+ !is_gl_identifier(consumer_var->name)) {
+const size_t name_len = strlen(consumer_var->name);
+
+if (name_len >= name_buffer_size) {
+   free(name_buffer);
+
+   name_buffer_size = name_len + 1;
+   name_buffer = malloc(name_buffer_size);
+   if (name_buffer == NULL) {
+  valid = false;
+  goto out;
+   }
+}
+
+consumer_name = (char *) name_buffer;
+
+char *s = strchr(consumer_var->name, '[');
+if (s == NULL) {
+   valid = false;
+   goto out;
+}
+
+char *t = strchr(s, ']');
+if (t == NULL) {
+   valid = false;
+   goto out;
+}
+
+assert(t[1] == '.' || t[1] == '[');
+
+const ptrdiff_t base_name_len = s - consumer_var->name;
+
+memcpy(consumer_name, consumer_var->name, base_name_len);
+strcpy(consumer_name + base_name_len, t + 1);
+ }
+
  for (unsigned j = 0; j < num_outputs; j++) {
 const gl_shader_variable *const var = outputs[j];
 
 if (!var->explicit_location &&
-strcmp(consumer_var->name, var->name) == 0) {
+strcmp(consumer_name, var->name) == 0) {
producer_var = var;
match_index = j;
break;
@@ -1529,25 +1583,53 @@ validate_io(struct gl_shader_program *producer,
* Note that location mismatches are detected by the loops above that
* find the producer variable that goes with the consumer variable.
*/
-  if (producer_var->type != consumer_var->type ||
-  producer_var->interpolation != consumer_var->interpolation ||
-  producer_var->precision != consumer_var->precision) {
+  if (nonarray_stage_to_array_stage) {
+ if (!consumer_var->type->is_array() ||
+ consumer_var->type->fields.array != producer_var->type) {
+valid = false;
+goto out;
+ }
+
+ if (consumer_var->interface_type != NULL) {
+if (!consumer_var->interface_type->is_array() ||
+consumer_var->interface_type->fields.array != 
producer_var->interface_type) {
+   valid = false;
+   goto out;
+}
+ } else if (producer_var->interface_type != NULL) {
+valid = false;
+goto out;
+ }
+  } else {
+ if (producer_var->type != consumer_var->type) {
+valid = false;
+goto out;
+ }
+
+ if (producer_var->interface_type !=